-
Notifications
You must be signed in to change notification settings - Fork 632
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
Limited Replayability Tracking Issue #8197
Comments
We discussed this in yesterday’s protocol meeting. The summary is as follows:
We overall agreed that:
|
We then discussed it again today in the contract runtime meeting. Our current plan is to:
|
A quick overview of existing traits / interface between the 3 runtime could be useful to inform a decision where a cut could be made. chain -> NightshadeRuntime
NightshadeRuntime <-> transaction runtime (
|
Corrections on the previous comments are welcome, I'm not sure I got this all correct. But based on what I described there, my opinion is that a clean API for the transaction runtime would probably be a cleaned-up version of what currently sits between |
So I just checked the current crate structure, and in order to publish crates on crates.io we would need to publish crates for near-vm-runner and all its dependency tree at least:
This makes me reconsider the idea of starting with manually publishing crates, even though I still think @nagisa’s idea from today’s meeting (which I’ll let you describe) is the best long-term. Maybe we’d be better off just copy-pasting near-vm-runner for today’s lowest-hanging-fruit goal? And then in a second step I think we should reintroduce the automated deploy-from-CI behavior, and get an auto-deploy of literally all crates with a major release on every nearcore version, ideally numbering that release with the latest protocol version this set of crates supports for clarity. It’d also make it easier for other teams to integrate limited replayability in their own workflows. As a side-note, I remember seeing people from the rust community angry at… was it solana? for doing exactly this kind of huge update-all-crates events regularly. So if we actually go with this solution we may want to sponsor even a moderate recurring amount to Rust Foundation, so that we could reasonably answer that we’re paying for the resources we’re using should people complain about it. @akhi3030 maybe that’s a thing you could look into? (or maybe we already do, which’d be awesome) |
So here's the idea we came up in our meeting today, which we think is in the short term is the best and easiest way forward.
The cost of this is that every time we publish a runtime, the size of the final neard binary will increase by the size of a single runtime binary (sans whatever LLVM manages to deduplicate.) Another cost is linearly increasing compilation time, which I think we can deal with until we get more time to work out something better.
I think ideally we make the runtime crate(s) self-contained as much as possible, reducing the number of crates the runtime is split into. There's no inherent reason why we need to maintain One observation is that building a |
There are some projects like that – this was handled pretty well by deprioritizing these crates in places like e.g. docs.rs queue where interactive uses of the service disproportionally suffer from added latency spikes. Otherwise publishing crates in patches is not a big deal AFAIU. |
Right, so we probably do need to plan on spending quite some time refactoring, as the API of near-vm-runner will probably be much larger than the
Right, I don’t think it’s a big deal either, but PR is not a rational thing and if Pagoda can avoid a PR hit (or even turn it into positive PR) for like $10/mo it’s probably worth it :) |
@Ekleog-NEAR @nagisa is the eventual goal to extend this idea to |
We had hoped this to be a nearcore-wide thing, however when discussing this idea with other teams the overall conclusion was that no other team is feeling the impact of replayability requirements as much as the contract runtime does. For them the resources-to-benefit ratio is significantly less than for the contract runtime, especially with how busy they are right now with stuff that's higher priority anyhow. As thus we're pursuing a design that the contract runtime team can implement on its own. Unfortunately, this also means that the solution is unlikely to be expandable past the runtime boundaries easily, or at least the end result wouldn't make much sense, holistically speaking. (This is my opinion, Leo’s may be different) |
To me one of the most important goals of limited replayability is to improve the maintainability of the codebase overall. Restricting our attention to the contract runtime will fall short of that goal. That said, I think it is okay to move gradually and potentially start with contract runtime first, but the design should be extendable to other parts of the codebase. cc @akhi3030 @jakmeier |
@bowenwang1996: I would also like to see this project improve the maintainability of the entire codebase. I think it is good to start with just runtime as the first milestone and then see where we are. |
As I remember the discussion, the major push back came because proposed solutions that work holistically requires multiple neard binary versions on validator nodes. (During protocol upgrade, they hot switch from one to the other. Very tricky to do with RocksDB, network state, and so one, considering the time to upgrade is only one block time.) But regardless of the strategy, it feel like the RocksDB layer of storage, and the network stack, have their own concepts of versioning that is not easily solvable in a general way. For those components, we have to think carefully about backwards compatibility on each upgrade anyway and keeping multiple versions of the code around might only make things worse. For the contract runtime OTOH, we can just include multiple versions in the same binary and pick them dynamically. This idea can be extended to the transaction runtime, if we can define appropriate interfaces. Those two components are also where we have most protocol version dependent if-branches at the moment. Note that this doesn't drop support for old protocol versions. It only makes runtime code cleaner and easier to maintain. |
@jakmeier the separation between what we do manually and what we implement programmatically makes sense. I guess one thing we need to consider is what to do in case there is a need to replay some old history with old versions. Today it is possible to do with the same binary, but if we manually remove some old versions then we need some other solution. |
The simplest solution would probably be to offload the work to those who actually want to replay a complete history. They would have to checkout old nearcore tags, compile them with the specified toolchain, and then use the different binary versions for different block height ranges. I'm not sure if anything more sophisticated is worth building. At least I am not aware of anyone actually doing such replays. |
I agree. I would wait and see if someone actually requests something more sophisticated before we devote resources to it. |
* fix(state-sync): finalize using flat storage * fix(state-sync): Applied values from state parts should be inlined when applicable (#9265) * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * clippy * fmt * logging * scan from a hash * refactor: add FlatStateValue constructor for on-disk values (#9269) This logic is used in multiple places, so it makes sense to abstract it behind a method in `FlatStateValue`. `on_disk` name is chosen over `new` since this logic is only applicable for values to be persisted on disk. Later we will also add inlining for values as part of Flat Storage Delta and those will have lower inlining threshold, so we will have another constructor. * refactor: Move adjust-db tool to database tool (#9264) Refactoring of `adjust-db` tool, moving it to the `database` toolset where there's of database tools should live. * feat: remove fixed shards (#9219) In this PR I'm removing the fixed shards field from the shard layout. I removed the fixed_shards field directly from the ShardLayoutV1. This implementation is quite hacky as it does not follow the typical flow for making protocol changes. This approach needs to be discussed and agreed on first but here is an implementation just to get a feel for it. The motivation for removing fixed shards is that fixes_shards break the account ids contiguity within a shard. A sub-account of a fixed shard is assigned to the same shard as the fixed account but it may fall in the middle of account range of another shard. For example let's consider the following shard layout: ``` fixed_shards = ['fixed'] boundary_accounts = ['middle'] ``` In this shard layout we have three shards: - 0 - 'fixed' and all sub-accounts - 1 - [..'middle'] with exception for any sub-account of fixed - 2 - ['middle'..] with exception for any sub-account of fixed Because of the fixed account, shards 1 and 2 are now full of holes for any subaccounts of 'fixed'. This property of fixed_shards makes it hard to perform any range operations on a shard such as resharding or deletion. The proper way to do it would be to add ShardLayoutV2 - a copy of V1 with fixed_shards removed and version bumped. Due to the way that we use for storing state date in storage - using the shard_uid as a prefix for the storage key - this approach would require a careful migration. One way to go about it would be to perform a null-resharding in epoch preceeding the shard layout version bump. There the node would have to create a copy of the state and store it in storage with the new version in shard_uid. Once the state is duplicated the node can keep applying changes to both states, same as in resharding. Such a migration would need to be prepared very carefully and would take a lot of time and effort. The reason why I believe that we can get away without new shard layout version is that fixed_shards are not used in production and the shard version is not actually stored in the state. The ShardLayoutV1 with empty fixed_shards is semantically equivalent to ShardLayoutV1 with the fixed_shards field removed. It's worth mentioning that the migration pains would be removed if we decided to change the storage structure to one that doesn't have the shard_uid in the storage key. * remove near-cache dep from near-vm-runner (#9273) Part of #8197 * fix(state-sync): Clear flat storage outside of ClientActor (#9268) Deleting multiple GBs from RocksDB is definitely taking more than 100ms. In case a node catches up, it can potentially make a chunk producer or a block producer miss its chunk/block. This PR moves deletion of Flat State to `SyncJobsActor` to avoid blocking `ClientActor`. * remove near-stable-hasher dep from near-vm-runner (#9270) Part of #8197 * fix(state-sync): Applied values from state parts should be inlined when applicable (#9265) * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * fix(state-sync): Applied values from state parts should be inlined when applicable * clippy * fmt * fix(state-sync): Finalize state sync using flat storage * fix(state-sync): Finalize state sync using flat storage --------- Co-authored-by: Anton Puhach <anton@near.org> Co-authored-by: Jure Bajic <jure@near.org> Co-authored-by: wacban <wacban@users.noreply.github.com> Co-authored-by: Ekleog-NEAR <96595974+Ekleog-NEAR@users.noreply.github.com> Co-authored-by: near-bulldozer[bot] <73298989+near-bulldozer[bot]@users.noreply.github.com>
One axis of consideration that I feel our summary of possible split points does not capture is where does the serialized forms of chain data materializes. Unfortunately it seems that right now we have a bunch of that all over the place. Some of it is in
Ideally, I feel, we should end up in a situation where all of these schemas are part of our limited replayability story, which would mean, I feel, that we'd need to pull in a bunch of |
Rather than keeping these types inside near-vm-runner where they don't belong conceptually and interfere with limited replayability due to being schemas for serialization on-chain, we move them to near_primitives and expose the necessary interfaces to the `near-vm-runner` via `Externals`. With this none of the near-vm-runner code is on-chain schema sensitive, which is one less thing to worry about. “But wait,” you ask “shouldn’t limited-replayability help with not needing to think about what changes are made in the first place?” Yes, indeed it should be that way, but it isn’t entirely clear to me if we don’t or won’t have intentional or accidental functionality where the data is not serialized, but rather deserialized, even without a need to invoke the runtime to execute the code. This might be as simple a tool as something that shows the stored blockchain structure (e.g. near explorer.) (But more seriously, I have petitioned that it would make sense to have all schemas part of the limited replayability story as well, but that didn't really gain as much traction as I had hoped, so I’m just punting this problem upwards and away from the runtime crates.) cc #8197
Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc #8197
Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc #8197
…eatures to runtime configuration (#9364) This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc @jakmeier cc #8197
Rather than keeping these types inside near-vm-runner where they don't belong conceptually and interfere with limited replayability due to being schemas for serialization on-chain, we move them to near_primitives and expose the necessary interfaces to the `near-vm-runner` via `Externals`. With this none of the near-vm-runner code is on-chain schema sensitive, which is one less thing to worry about. “But wait,” you ask “shouldn’t limited-replayability help with not needing to think about what changes are made in the first place?” Yes, indeed it should be that way, but it isn’t entirely clear to me if we don’t or won’t have intentional or accidental functionality where the data is not serialized, but rather deserialized, even without a need to invoke the runtime to execute the code. This might be as simple a tool as something that shows the stored blockchain structure (e.g. near explorer.) (But more seriously, I have petitioned that it would make sense to have all schemas part of the limited replayability story as well, but that didn't really gain as much traction as I had hoped, so I’m just punting this problem upwards and away from the runtime crates.) cc #8197
Rather than keeping these types inside near-vm-runner where they don't belong conceptually and interfere with limited replayability due to being schemas for serialization on-chain, we move them to near_primitives and expose the necessary interfaces to the `near-vm-runner` via `Externals`. With this none of the near-vm-runner code is on-chain schema sensitive, which is one less thing to worry about. “But wait,” you ask “shouldn’t limited-replayability help with not needing to think about what changes are made in the first place?” Yes, indeed it should be that way, but it isn’t entirely clear to me if we don’t or won’t have intentional or accidental functionality where the data is not serialized, but rather deserialized, even without a need to invoke the runtime to execute the code. This might be as simple a tool as something that shows the stored blockchain structure (e.g. near explorer.) (But more seriously, I have petitioned that it would make sense to have all schemas part of the limited replayability story as well, but that didn't really gain as much traction as I had hoped, so I’m just punting this problem upwards and away from the runtime crates.) cc near#8197
…eatures to runtime configuration (near#9364) This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc @jakmeier cc near#8197
Rather than keeping these types inside near-vm-runner where they don't belong conceptually and interfere with limited replayability due to being schemas for serialization on-chain, we move them to near_primitives and expose the necessary interfaces to the `near-vm-runner` via `Externals`. With this none of the near-vm-runner code is on-chain schema sensitive, which is one less thing to worry about. “But wait,” you ask “shouldn’t limited-replayability help with not needing to think about what changes are made in the first place?” Yes, indeed it should be that way, but it isn’t entirely clear to me if we don’t or won’t have intentional or accidental functionality where the data is not serialized, but rather deserialized, even without a need to invoke the runtime to execute the code. This might be as simple a tool as something that shows the stored blockchain structure (e.g. near explorer.) (But more seriously, I have petitioned that it would make sense to have all schemas part of the limited replayability story as well, but that didn't really gain as much traction as I had hoped, so I’m just punting this problem upwards and away from the runtime crates.) cc #8197
…eatures to runtime configuration (#9364) This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc @jakmeier cc #8197
A point for myself is to move this forward is to:
|
ScopeBefore this message the scope of this task was quite ambiguous. We were not sure if we want a split to be entirely at the boundary of the Well, it turns out that the ambiguity has caused us more harm than good, actually. The progress itself had to be put on hold for us to rethink the approach here. Eventually we realized that we should focus on putting out a working MVP prototype. Once we have a working MVP we can then plan our future steps without making this a huge change that touches upon everything in the system. Note that this explicitly does not include into the limited replayability scheme some of the very stable common dependencies – For the MVP lets depend on Fortunately there shouldn’t be too much remaining to achieve all of this. Remaining work
|
I’ll ask, for the remaining dependencies of near-vm-runner: do we want to stabilize them, releasing a 1.0 for them like near-account-id, to disincentivize ourselves against making breaking changes to them? |
The three dependencies that we have currently are We clearly cannot freeze
|
This is not currently being worked on, so unassigning. |
Today any development to neard is required to maintain the exact behavior as seen in all past versions of the program. Among other things, this is necessary to enable replay of the blockchain all the way back to the genesis, as implemented by the various neard view-state sub-commands.
Some of the prerequisite infrastructure to reconcile the operational needs and the desire to modify the protocol already exists in the form of protocol versioning. Practice has nevertheless demonstrated protocol versioning to incur a significant hit to the development velocity. Every time a change is made, engineers need to carefully consider the interaction between the change and protocol-visible behaviour. In cases where the change is intentionally a breaking change to the protocol, care needs to be taken to also maintain the former behaviour for the previous version of the protocol. In extreme cases the only feasible way to maintain compatibility is to duplicate the affected subsystem in full. First time this happens, logic to switch between different versions of the functionality must be implemented as well. Such logic acts to further impede future evolution of the implementation.
We are not able to consistently verify whether our efforts to maintain compatible behaviour are being implemented correctly in practice. Verifying the protocol versioning is implemented correctly by replaying even portions of the history is an extremely time-consuming process (taking weeks to months per experiment) and requires significant effort to set up. This makes verification of code changes quite onerous, to the point where there has only been One Attempt on behalf of the Contract Runtime team back in 2021.
It would be ideal, if we could make the requirements for compatibility satiable by construction and remove the burden imposed by this functionality on the development process.
See the proposal thread for further details.
Current status
The text was updated successfully, but these errors were encountered: