-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a feature that propose u32
tx_pointer
in fuel-tx
and fuel-vm
#899
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but in the malleable_fields_do_not_affect_*
tests if we used u16::MAX and u32::MAX to indicate change in size vs adding some zeroes OR using rng to generate those values, would be nice, but is out of scope for this PR :)
It is a big breaking change for the type system. We need to have proper testing and double check that serialization and deserialization will be not affected for mainnet/testnet/devnet blocks. It would be nice try to use extreme values and see when postcard serialization/deserialiation will be different(like from 1 to u16::MAX). Also maybe Rollup team uses bitcode, if yes, then it also break compressed blocks for them |
@xgreenx I didn't want to activate this feature on Ignition is it something we want to do in the near future ? For xxx, it will be a new network and will be in the specs. |
We can activate it on Ignition, if tests show that old database types are compatible with it. |
I have added a test to showcase that we have a difference on the postcard serialization format (it's expected because postcard use varint encoding). I tried our I don't really understand how we could activate this on Ignition but if we want to make more tests to activate this for Ignition they should be done on the |
The answer for this is:
Because we use postcard in P2P and Database, the variant encoding of integers should help us to deserialize old types of transactions, because we know that tx pointer is not more than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with this change would be nice to introduce "block_max_transactions" parameter to ConsensusParameters
. It can be a separate change, we just need these two things released together.
Added in 9273970 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, would be nice for some fuzzing/proptest for this new change
It should be a compatibility test where we use the previous release(published crate) of the |
ca01d89
to
dd3a7ea
Compare
Thanks for the detailed explanation @xgreenx. Done :) |
Co-authored-by: Green Baneling <[email protected]>
let tx_pointer = fuel_tx_0_59_1::TxPointer::new(1u32.into(), idx); | ||
let expected = latest_fuel_tx::TxPointer::new(1u32.into(), idx.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask you to do the same for script transaction please?=D Or Mint
transaction for simplicity
Having
u32
tx_pointer
is mendatory to make our system scales and support more thanu16::MAX
tx in a block.However, changing this in the current network is a hard breaking change and is not needed for now. That's why we decided to add this change to the VM behind a feature flag if we want to use in other system or be able to enable it in our principal one too.
Checklist
Before requesting review