asset-security -> staging

[mat-if]

Summary

This is the PR to merge all the asset-security changes into staging. Every PR into the asset-security branch was code-reviewed already, but this gives a nice opportunity to see it all together.

A few remaining tasks remain around the RPC, CLI and APIs, and updating the DBs, but this PR contains all the code necessary to handle v2 transactions and the hardfork transition from v1 to v2

Testing Plan

Unit tests
Manual testing

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.

[ ] Yes

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[x] Yes

This lays the foundation to enable a consensus parameter for v2 transactions and asset ownership transfers. Will need to document all the breaking changes collectively in an easy to digest format for other developers in the ecosystem

[ygao76]

Can we pick the activation date and then merge to staging? I know currently the sequence is safe, but it does not feel 100% right to have this placeholder value in staging, because if we need to do a release from the staging before the date is updated, that code will be released. I know the consequence is just user need to update to latest version before hard fork, which they need to do it anyway, but it still does not feel right to release a version with placeholder value, especially we can easily avoid it.

On the other note, I think this feature should be tested e2e before merging to staging, not testing it while on staging just in case there is a release from staging before we can confidently release it. Is there any concern having it on the feature branch developing and testing it?

[andreacorbellini]

Can we pick the activation date and then merge to staging? I know currently the sequence is safe, but it does not feel 100% right to have this placeholder value in staging, because if we need to do a release from the staging before the date is updated, that code will be released. I know the consequence is just user need to update to latest version before hard fork, which they need to do it anyway, but it still does not feel right to release a version with placeholder value, especially we can easily avoid it.

If there's a concern over using dummy placehoders, then we can use a special purpose value. Example: change the type of the activation sequence from number to number | undefined or even more explicitly to number | 'never', and then use 'never' as the placeholder value, so it's more clear what the intention is.

On the other note, I think this feature should be tested e2e before merging to staging, not testing it while on staging just in case there is a release from staging before we can confidently release it. Is there any concern having it on the feature branch developing and testing it?

Making small and frequent commits to a shared branch (staging in this case) allows for a more agile work environment. Working on separate branches that accumulate big changes for several weeks is an approach that tends to be very time consuming. Often merge conflicts that are not easy to solve arise, and those can cause unexpected shifts in delivery dates. Making small and frequent changes to the main development branch is the same philosophy behind "continuos delivery", which is proven to be an effective approach for software development.

This approach comes with the price that sometimes the main development branch (stagingin this case) may contain incomplete changes, but that's fine as long as there's no way to activate that logic.

That's just my opinion though—happy to hear a different opinion on this topic.

[ygao76]

Can we pick the activation date and then merge to staging? I know currently the sequence is safe, but it does not feel 100% right to have this placeholder value in staging, because if we need to do a release from the staging before the date is updated, that code will be released. I know the consequence is just user need to update to latest version before hard fork, which they need to do it anyway, but it still does not feel right to release a version with placeholder value, especially we can easily avoid it.

If there's a concern over using dummy placehoders, then we can use a special purpose value. Example: change the type of the activation sequence from number to number | undefined or even more explicitly to number | 'never', and then use 'never' as the placeholder value, so it's more clear what the intention is.

On the other note, I think this feature should be tested e2e before merging to staging, not testing it while on staging just in case there is a release from staging before we can confidently release it. Is there any concern having it on the feature branch developing and testing it?

Making small and frequent commits to a shared branch (staging in this case) allows for a more agile work environment. Working on separate branches that accumulate big changes for several weeks is an approach that tends to be very time consuming. Often merge conflicts that are not easy to solve arise, and those can cause unexpected shifts in delivery dates. Making small and frequent changes to the main development branch is the same philosophy behind "continuos delivery", which is proven to be an effective approach for software development.

This approach comes with the price that sometimes the main development branch (stagingin this case) may contain incomplete changes, but that's fine as long as there's no way to activate that logic.

That's just my opinion though—happy to hear a different opinion on this topic.

I agree with what you said about agile development, for most of change I would def go for merging small changes. However, with asset secuirity, since it contains the consensus changes and affects all stacks of our code base, I would not recommend we go with the agile flow to the staging branch. We have been always doing the feature branch for big features that involves rust side changes or consensus changes, previously @mat-if also did it for assets and blstrs. Additionally, we have the standalone wallet team that likely to do release from staging for their feature release or hot fix, it's better to not merge this feature in staging for now.

[mat-if]

It's important context to this discussion to consider why multi-asset was a long-lived feature branch. blstrs was not really a feature branch in the same sense. It was really just a regular PR.

Multi-asset was a huge, sweeping change from the CLI down to the circuits and parameters. We had no concept of sequence activating consensus parameters because it was pre-mainnet and we did not care to support such a case when we'd just revert any logic built around that when we launched mainnet.

These changes are mostly just around supporting V2 transactions in the code base properly.

A good portion of the changes required for this project are already out in versions 1.7, 1.8 and 1.9. This branch mostly existed because there was a set of changes that needed to happen to properly support the transaction versioning that couldn't merge piece-by-piece. The alternative was a very large, un-reviewable PR.

[ygao76]

@mat-if I thnk essentially we are talking about the same plan, having this asset-security branch for changes that can not merge to staging one by one. Correct me if I am wrong, my understanding is when this branch is ready to merge to staging. It also means asset security is almost code complete. At that point, we can confidently pick a block sequence as activation date. All I am suggesting is have this sequence number right before merging to staging. I dont think it will cause huge delay or any additional merging conflicts.

I was trying to explain to @andreacorbellini its common to have a feature branch for features that touches all code base and working against this feature branch instead of staging. Specifically, I am responding to this

Making small and frequent commits to a shared branch (staging in this case) allows for a more agile work environment.

Andrea feel free to add more details if you are not mean to oppose the feature branch. From the message, my understanding is you dont like having this feature branch.

[andreacorbellini]

Yes, I'd like this feature branch to be merged in staging as soon as possible. We're spending too much time rebasing and resolving conflicts, especially because there's another major project (standalone wallet) which touches some code that is shared with assets security.

A final block sequence won't be decided any time soon ("soon" == "by the end of this week"), which means that if we don't merge this, we will have to keep rebasing and resolving conflicts next week too, even though the work is in theory complete.