feat(consensus): add L2 contracts VM storage reader (BFT-434) #2416

moshababo · 2024-07-10T05:40:25Z

What ❔

Adding eth_call-based storage reader for consensus L2 contracts.

Why ❔

Enable consensus executor to read from L2 storage per specific block.

Notes

consensus::storage::VMReader added to incapsulate read-transactions dispatching against L2 storage.
consensus::storage::testonly::VMWriter added in order to simulate contracts deployments and data initialization in unit tests.
consensus::storage::tests::test_vm_reader() test added.
Multiple visibility modifiers were changed in order to facilitate the new codebase dependency graph.

TODOs

Remove contracts git submodule custom branch reference once feat(consensus): add L2 registry contract (BFT-434) era-contracts#555 is merged.

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zk fmt and zk lint.

popzxc

A few high-level comments. I understand that the PR is still in draft, but the comments are mostly about design.

popzxc · 2024-07-10T09:17:08Z

core/node/consensus/src/storage/vm_reader.rs

+    consensus_authority_contract: Contract,
+    validator_registry_address: Option<Address>,
+    validator_registry_contract: Contract,
+    attester_registry_address: Option<Address>,
+    attester_registry_contract: Contract,


Do we need 3 dedicated contracts for consensus? Our contracts are already very sophisticated, so my fear is that it would complicate them even more.
I do not have concrete proposals, but I strongly suggest discussing the contracts design with @vladbochok and @StanislavBreadless

Not actionable in this PR I guess.

That's my fault. I did the design for the contracts and thought that smaller contracts would be preferred. They can be merged into a single larger contract though.

Changes to contracts here: matter-labs/era-contracts@0c2498f

popzxc · 2024-07-10T09:19:46Z

core/node/consensus/src/storage/vm_reader.rs

+        let consensus_authority_contract = load_contract("contracts/l2-contracts/artifacts-zk/contracts/ConsensusAuthority.sol/ConsensusAuthority.json");
+        let validator_registry_contract = load_contract("contracts/l2-contracts/artifacts-zk/contracts/ValidatorRegistry.sol/ValidatorRegistry.json");
+        let attester_registry_contract = load_contract("contracts/l2-contracts/artifacts-zk/contracts/AttesterRegistry.sol/AttesterRegistry.json");


zksync_contracts crate currently serves (somewhat) as a schema for our FS dependencies, so probably it would be better to load the contracts there.
You can create a module for consensus_l2_contracts and load them via pub fn load_consensus_l2_contracts() -> Option<ConsensusContracts>.

Fixed here: 5a925c6

core/node/consensus/src/storage/vm_reader.rs

popzxc · 2024-07-10T09:28:43Z

core/node/consensus/src/storage/vm_reader.rs

+        let num_committee_validators = self.read_num_committee_validators(ctx, block_id).await;
+        for i in 0..num_committee_validators {
+            let committee_validator = self.read_committee_validator(ctx, block_id, i).await;
+            let validator = self
+                .read_validator(ctx, block_id, committee_validator.0)
+                .await;


You seem to instantiate a new DB connection and VM instance for each read request, and it's a somewhat expensive operation. (Actually likely several ones, because tx_sender would also create connections internally).

If we want to actually run the VM, I'd expect us to do it efficiently, roughy:

let vm = self.instantiate_vm().await; let num_committee_validators = self.read_num_committee_validators(&vm, block_id).await; // ^ run on an already instantiated VM. // ... same for the rest of the function.

Yes, it was just a PoC.

It was fixed in a8779b5, so that only one connection will be acquired per read session. It does not however (yet) re-use that connection within the execution sandbox. The connection is only used to load block args.

popzxc · 2024-07-10T09:30:26Z

core/node/consensus/src/storage/vm_reader.rs

+    ) -> (usize, Vec<u8>, Vec<u8>, bool) {
+        let func = self
+            .validator_registry_contract
+            .function("validators")
+            .unwrap()
+            .clone();
+        let tx = self.gen_l2_call_tx(
+            self.validator_registry_address.unwrap(),
+            func.encode_input(&[Token::Address(node_owner)]).unwrap(),
+        );
+        let res = self.eth_call(ctx, block_id, tx).await;
+        let tokens = func.decode_output(&res).unwrap();
+        (
+            tokens[0].clone().into_uint().unwrap().as_usize(),
+            tokens[1].clone().into_bytes().unwrap(),
+            tokens[2].clone().into_bytes().unwrap(),
+            tokens[3].clone().into_bool().unwrap(),
+        )
+    }


Probably it would be better to implement zksync_types::web3::contract::Tokenize on all the required structures instead of mixing parsing with the business logic.

Changes here: 477ea5e

popzxc · 2024-07-10T09:33:49Z

core/node/consensus/src/storage/vm_reader.rs

+        let block_args = zksync_node_api_server::execution_sandbox::BlockArgs::new(
+            &mut conn,
+            block_id,
+            &start_info,
+        )
+        .await
+        .unwrap();
+        let call_overrides = CallOverrides {
+            enforced_base_fee: None,
+        };
+
+        let res = self
+            .tx_sender
+            .eth_call(block_args, call_overrides, tx)
+            .await
+            .unwrap();


If we want to consider running VM to query contracts (as opposed to pre-calculating storage slots and reading storage directly), I think we need to do quite some preparation work:

Extract execution_vm into a separate, general-purpose crate.

Change the sandbox so that it can perform multiple requests on a single instance.

Decouple it from TxSender.

Right now, tx_sender and dependency on zksync_node_api_server feel like a pretty huge hack, and I'm not sure if it's a good idea to depend on that.

I can certainly see pros of using VM with eth_call, but I believe that implementing it properly would take a lot of engineering efforts. As you can see, the first try is implemented in a hacky way, and making a proper implementation (e.g. detaching sandbox from API) is a complex task even for seasoned platform developers. I would really like to not use hacky ways, as they may backfire, especially given that consensus will be a critical component for the sequencer.

On the other hand, reading storage directly will never look as "good", but it's relatively easy to implement and cover with unit tests (e.g. change value via contract method, then read from storage directly) to make sure that the storage layout doesn't change.

If we can afford to spend time on extracting the sandbox from API, I'm all for it. If not, I would rather prefer direct storage access than hacks.

I agree that the current solution is a workaround/hack as per the code dependency graph and the lack of execution sandbox instance re-use.

However, falling back to direct storage reads is worse IMHO.

The reads are not targeting U256 variables in which their storage slots’ keys can be easily pre-calculated, be fetched and decoded. It involves dynamic-sized arrays, structs, and dynamic-sized arrays within the structs. This implies packing/padding, which means to re-implement the VM codec.

Meanwhile, I couldn’t find any relevant documentation, and there’s very limited code examples (mostly in zksync_types::utils), for much simpler use-cases.

I went ahead to experiment with it, and couldn’t even retrieve a simple U256 variable slot, following canonical solc storage layout. I might have overlooked something, but couldn’t reverse-engineer the slots back to the pre-hashed keys. In the overall, I don’t think it’s a good idea to go forward with this given the complex data types involved.

Also, I don’t think such approach is testable, because the layout is supposedly an implementation detail, unless there’s a clear open available specs, backwards compatibility, and lack of potential optimizations.

Read-transactions are the standard way to read from the VM storage database, while incapsulating the layout and the codec. The consensus component functions as a standard web3 client in this regard, and having direct access to the database does not imply it should use it to bypass the standard interface.

I suggest to continue with the current approach, and to implement your suggestions for the execution sandbox decoupling, in a follow-up PR. (see BFT-493)

+1. That was also the protocol teams's opinion, that we should avoid direct storage reads if at all possible.

I don't mind as long as refactoring will really be planned & executed.

popzxc · 2024-07-22T05:41:48Z

.gitmodules

+url = https://github.com/matter-labs/era-contracts
+branch = consensus_contracts


JFYI: I don't think the PR is mergeable until we can merge the contracts to main (e.g. we cannot live with a separate branch in submodules).

I think we should start a discussion with the protocol team on how to merge these changes to main without having to finish/audit them, and meanwhile minimize risks of them somehow affecting production environments.

I have a few ideas, let's discuss in Slack. cc @RomanBrodetski @brunoffranca

core/node/consensus/src/storage/testonly.rs

core/node/consensus/src/storage/vm_reader.rs

popzxc · 2024-07-22T05:57:46Z

core/node/consensus/src/storage/vm_reader.rs

+        self.tx_sender
+            .eth_call(block_args, call_overrides, tx)


Continuing the discussion from this comment.
tx_sender.eth_call will instantiate a connection, then VM, fetch a bunch of stuff from Postgres, and only then perform an eth_call. So e.g. in read_attester_committee method above you will create attester_committee_size + 1 connections sequentially, with quite some overhead.
I'm still concerned that the performance here may be suboptimal.

Yes, connections are acquired per-call by the execution sandbox. This is indeed suboptimal, and should be part of the needed refactoring. (BFT-493)

popzxc · 2024-07-22T05:59:42Z

core/node/consensus/src/storage/vm_reader.rs

+/// A struct for reading data from consensus L2 contracts.
+pub struct VMReader {
+    pool: ConnectionPool,
+    tx_sender: TxSender,


A general observation, not sure if it's a concern. TxSender uses replica DB to read state, so there are a few problems here:

Replication lag may slow the consensus down.

Replica DBs are used by API, and the load there is much higher, so the latency may be less predictable than with main DB.

I guess this can be "quickly solved" by pointing replica DB url to main DB on the components where core runs, but it feels even more of a hack.

Once execution sandbox will be decoupled, we could use it with main DB connection pool, and no changes to TxSender would be required.

pompon0 · 2024-07-29T14:19:36Z

core/node/consensus/src/storage/testonly.rs

@@ -1,18 +1,24 @@
 //! Storage test helpers.

 use anyhow::Context as _;
-use zksync_concurrency::{ctx, error::Wrap as _, time};
+use zksync_concurrency::{ctx, ctx::Ctx, error::Wrap as _, time};


nit: don't import ctx::Ctx, use the qualified name.

pompon0 · 2024-07-29T14:20:31Z

core/node/consensus/src/storage/testonly.rs

+        &mut self,
+        ctx: &Ctx,
+        owner: Address,
+        nodes: &[&[Token]],


can the conversion be done inside the function, passing &[&[Token]] is very loosely typed.

pompon0 · 2024-07-29T14:22:53Z

core/node/consensus/src/storage/testonly.rs

+            .function("setAttesterCommittee")
+            .unwrap()
+            .short_signature()
+            .to_vec();


all these methods wrapping raw contract API deserve a dedicated module.

aakoshh · 2024-08-13T23:19:22Z

core/node/consensus/src/storage/testonly.rs

+            let tx = self.gen_tx_add(&registry_contract.contract, deploy_tx.address, node);
+            txs.push(tx);
+        }
+        txs.push(
+            self.gen_tx_set_validator_committee(deploy_tx.address, &registry_contract.contract),


Is there any rason why the contract and the address parameters are in opposite order?

aakoshh · 2024-08-13T23:26:31Z

core/node/consensus/src/storage/testonly.rs

+        contract: &Contract,
+    ) -> Transaction {
+        let calldata = contract
+            .function("setValidatorCommittee")


I can't see this method in matter-labs/era-contracts#555 ; is it commitValidators ?

aakoshh · 2024-08-13T23:30:54Z

core/node/consensus/src/storage/tests.rs

+                U256::from(rng.gen::<usize>()),
+                (0..256).map(|_| rng.gen()).collect::<Vec<u8>>(),
+            )
+                .into_tokens();


Maybe you could use abigen! to generate bindings for all the Solidity structs and then you get the tokenization with static typing.

We didn't migrate to ethers just yet (and in fact it's already deprecated in favor of alloy), so using it is not recommended for now. We want the migration to be done consistently, otherwise, we'll end up with a mix of our vendored web3 remains and some of the newer stuff.

aakoshh · 2024-08-13T23:58:37Z

core/node/consensus/src/storage/vm_reader.rs

+        let func = self
+            .registry_contract
+            .function("validatorCommitteeSize")
+            .unwrap()
+            .clone();
+
+        let tx = self.gen_l2_call_tx(self.registry_address, func.short_signature().to_vec());
+
+        let res = self.eth_call(block_args, tx).await;
+
+        let tokens = func.decode_output(&res).context("decode_output()")?;
+        U256::from_tokens(tokens)
+            .context("U256::from_tokens()")
+            .map(|t| t.as_usize())


Would it be feasible to do a bit more encapsulation? For example to create a DeployedContract or similar type which has an Address and a Contract, then wrap that into a RegistryContract which has methods for these that take whatever is needed to do the eth_call but otherwise know which function to call and how to parse the result? Maybe we can use the abigen! macro to generate the bindings if the ABI JSON is available at compile time?

The VMReader sounds more general than just having all these methods for this particular contract, even if it's the only one at the moment.

I did something similar here where a gateway contract implemented multiple facets, each with their own binding, each represented by a contract caller that took the equivalent of the TxSender as an input. I suppose that gateway is similar to the VMReader, but a lot of the boilerplate and the private methods here could be delegated to contract specific structs, leaving only the highest level stuff in the reader.

aakoshh · 2024-08-14T00:04:19Z

core/node/consensus/src/storage/vm_reader.rs

+        let tx = self.gen_l2_call_tx(
+            self.registry_address,
+            func.encode_input(&zksync_types::U256::from(idx).into_tokens())
+                .unwrap(),


These unwraps could be at least expect with the method name to give us some context if they fail, in case we can't use bindings.

aakoshh · 2024-08-14T00:06:47Z

core/node/consensus/src/storage/vm_reader.rs

+        let func = self
+            .registry_contract
+            .function("attesterCommitteeSize")
+            .unwrap()


A contract wrapper could provide a method to get the function by name and make sure if it's not found the error explains what it was looking for.

aakoshh · 2024-08-14T00:12:51Z

core/node/consensus/src/storage/vm_reader.rs

+    pub pop: Vec<u8>,
+}
+
+impl Detokenize for CommitteeValidator {


There is an attribute in ethers that should work here even without generating the bindings. Here's an example.

pompon0 · 2024-08-19T15:34:12Z

closing this pr, I'll continue this work in #2684

Add consensus L2 contracts VM storage reader

e9b7c36

popzxc reviewed Jul 10, 2024

View reviewed changes

brunoffranca self-requested a review July 10, 2024 12:13

brunoffranca changed the title ~~[WIP] Add consensus L2 contracts VM storage reader~~ [WIP] Add consensus L2 contracts VM storage reader (BFT-434) Jul 10, 2024

moshababo added 8 commits July 12, 2024 14:49

Upgrade to new contract format

5a1a259

Use a single connection per read batch

a8779b5

Use abi (de)serialization traits, propagate errors

477ea5e

zksync_contracts to incapsulate FS

5a925c6

Add docs

f7b1b9b

Fix lint warnings

5680805

Add cargo deps

42a974c

Merge branch 'main' into consensus_contracts_reader

2ad2c32

moshababo changed the title ~~[WIP] Add consensus L2 contracts VM storage reader (BFT-434)~~ Add consensus L2 contracts VM storage reader (BFT-434) Jul 22, 2024

moshababo marked this pull request as ready for review July 22, 2024 00:22

moshababo changed the title ~~Add consensus L2 contracts VM storage reader (BFT-434)~~ feat(consensus): Add L2 contracts VM storage reader (BFT-434) Jul 22, 2024

moshababo changed the title ~~feat(consensus): Add L2 contracts VM storage reader (BFT-434)~~ feat(consensus): add L2 contracts VM storage reader (BFT-434) Jul 22, 2024

popzxc reviewed Jul 22, 2024

View reviewed changes

moshababo added 4 commits July 22, 2024 13:32

Add Debug derivation

899f07b

Add docs

d4e6f84

Propagate connection error

cbc46a1

Merge branch 'refs/heads/main' into consensus_contracts_reader

f1cda3e

moshababo mentioned this pull request Jul 29, 2024

[WIP] feat(consensus): committees data extractor (BFT-443) #2518

Closed

6 tasks

pompon0 reviewed Jul 29, 2024

View reviewed changes

aakoshh reviewed Aug 13, 2024

View reviewed changes

aakoshh reviewed Aug 14, 2024

View reviewed changes

pompon0 closed this Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(consensus): add L2 contracts VM storage reader (BFT-434) #2416

feat(consensus): add L2 contracts VM storage reader (BFT-434) #2416

moshababo commented Jul 10, 2024 •

edited

Loading

popzxc left a comment

popzxc Jul 10, 2024

brunoffranca Jul 10, 2024

moshababo Jul 10, 2024

popzxc Jul 10, 2024

moshababo Jul 21, 2024

popzxc Jul 10, 2024

moshababo Jul 21, 2024

popzxc Jul 10, 2024

moshababo Jul 21, 2024

popzxc Jul 10, 2024

popzxc Jul 10, 2024

moshababo Jul 21, 2024

brunoffranca Jul 22, 2024 •

edited

Loading

popzxc Jul 23, 2024

popzxc Jul 22, 2024

popzxc Jul 22, 2024

moshababo Jul 22, 2024

popzxc Jul 22, 2024

moshababo Jul 22, 2024

pompon0 Jul 29, 2024

pompon0 Jul 29, 2024

pompon0 Jul 29, 2024

aakoshh Aug 13, 2024

aakoshh Aug 13, 2024

aakoshh Aug 13, 2024

popzxc Aug 14, 2024

aakoshh Aug 13, 2024 •

edited

Loading

aakoshh Aug 14, 2024

aakoshh Aug 14, 2024

aakoshh Aug 14, 2024

pompon0 commented Aug 19, 2024

		url = https://github.com/matter-labs/era-contracts
		branch = consensus_contracts

feat(consensus): add L2 contracts VM storage reader (BFT-434) #2416

feat(consensus): add L2 contracts VM storage reader (BFT-434) #2416

Conversation

moshababo commented Jul 10, 2024 • edited Loading

What ❔

Why ❔

Notes

TODOs

Checklist

popzxc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoffranca Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pompon0 commented Aug 19, 2024

moshababo commented Jul 10, 2024 •

edited

Loading

brunoffranca Jul 22, 2024 •

edited

Loading

aakoshh Aug 13, 2024 •

edited

Loading