Handle new state transition bytecode and new consensus parameter version

[acerone85]

on on upgrade transaction

Closes #2584

Questions for the reviewers:

• I am assuming that the latest known state transition bytecode version and latest known consensus parameters versions will be loaded from the storage when the global root service starts, and will be passed to the proces_block function through the UpdateMerklizedTables trait. Is this reasonable?
• I am revalidating the upgrade transaction metadata, but I am not sure if this is needed?
• I am assuming that both state transition bytecode version and consensus parameters version increment by 1 when an upgrade transaction with the corresponding purpose is processed, is this correct?

Linked Issues/PRs

Description

Checklist

• Breaking changes are clearly marked as such in the PR description and changelog
• New behavior is reflected in tests
• The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

• I have reviewed the code myself
• I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

• Rust SDK
• Sway compiler
• Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
• Someone else?

[netrome]

Generally looks good to me. I'd suggest rebasing this on top of the test suite PR #2598 and add tests before merging.

[netrome]

Where do we use the IterableStore? I only see inserts currently

[acerone85]

here:

fuel-core/crates/fraud_proofs/global_merkle_root/storage/src/update.rs

Line 132 in 453819e

let keys_iterator =

[netrome]

Replied in the other thread, I see that IterableStore is a requirement for GetMerkleizedTablesParameters, but we don't seem to use that trait currently - so I think we could drop this requirement for now.

[acerone85]

ah yes, no need on UpdateMerkleizedTables. Removed in c8e6774.

[netrome]

Where do we use this trait? Or is the intention to use this to get the latest versions to feed to the update functions later?

[acerone85]

This is where I want to check that I got the required storage trait bounds correctly. The Idea is that we'll have something akin to the GenericDatabase to iterate over keys.

There is a blanket implementation of the trait for anything that implements both KeyValueInspect<Column = Column> + IterableStore<Column = Column>. GenericDatabase<RocksDB<Description: DatabaseDescription>> satisfies the trait bounds, so it seems like a reasonable choice.

[netrome]

Sure, I just mean that this trait doesn't seem to be used anywhere. I.e. there's no code currently calling get_latest_consensus_parameters_version and get_latest_state_transition_bytecode_version. Do you picture these methods to be called by the binary and then added to the UpdateMerkleizedTransaction before executing the process_block function?

[netrome]

Also as a nit I'd consider either moving this trait to a separate module or pushing it down a bit in the file, as UpdateMerklizedTablesTransaction::process_block is the main entry point of the update module and should be declared first imo.

[acerone85]

Agreed, we need to find a strategy about how we want to handle traits. I am afraid that with more "process" functions and more transaction types to handle, we will over-rely on traits to extend the functions available to a given struct.

[netrome]

Sure, hopefully we shouldn't require much more than these extra functions beyond the process_block function.

The base branch was changed.

[rymnc]

@acerone85 i think after the base branch was changed you need to cherry-pick specific commits and then rebase and force push :) , lgtm though happy to approve after its done

[acerone85]

@acerone85 i think after the base branch was changed you need to cherry-pick specific commits and then rebase and force push :) , lgtm though happy to approve after its done

As much as I am all for rebase over merge, once another branch has been merged into the working branch, I prefer working with merge. In this case git merge master did the trick.

[netrome]

As much as I am all for rebase over merge, once another branch has been merged into the working branch, I prefer working with merge. In this case git merge master did the trick.

As much as I respect people having different preferences, I personally don't think it's too much of a hassle to maintain stacked PRs while rebasing and the resulting version history is just so much easier to navigate. I typicall do git rebase -i master whenever the base branch has been merged, in which case it is very easy to clean up the version history and remove commits that have been squashed in the base PR. Cherry picking also works well if you prefer that.

While I agree that merge is typically quicker¹, a rebased branch is easier to read which saves time in the long run. It makes reviews much easier as well.

Case in point, the current PR currently has 43 commits to be compared against master (many inherited from the base PR).

Whereas if you look at this PR which is very similar (although slightly simpler) which was rebased just has three commits which are easy to interpret, providing a simpler context for reviewers:

Footnotes

1. If you don't have conflicts with the target branch, as soon as you have conflicts I think they are much easier to resolve if you have a well-maintained linear history that hasn't been messed up by previous merges. ↩

[acerone85]

As much as I am all for rebase over merge, once another branch has been merged into the working branch, I prefer working with merge. In this case git merge master did the trick.

As much as I respect people having different preferences, I personally don't think it's too much of a hassle to maintain stacked PRs while rebasing and the resulting version history is just so much easier to navigate.

I also use a rebase heavy workflow, especially for stacked PRs. However, in the same moment that I get a merge commit in my branch, I switch to a merge-based workflow because merge commits during rebases are a pain.

For me, when I am working on a stacked PR that targets a branch target_branch, and the latter changes on the origin, the following works very well:

git fetch origin 
git rebase --onto origin/target-branch target_branch 
# solve conflicts, finish rebase
git branch -f target_branch origin/target_branch

When the branch origin/target_branch gets merged, git rebase --onto origin/master target_branch works.

(The same workflow works very well also if you have several PRs stacked on top of each other).

[netrome]

...However, in the same moment that I get a merge commit in my branch, I switch to a merge-based workflow because merge commits during rebases are a pain.

Yeah this is why I try to avoid merges as much as possible 😅. I only update my PRs by merging the base branch if it's a large ongoing review, and I know the reviewers prefer the merge variant. But then I'm basically stuck with merges until the PR is completed, which can turn quite painful if more changes are requested and the base keeps updating.

[netrome]

LGTM, just a merge artifact that would be nice to clean up but we can do that in another PR as well.