ENG-326: Full Node Commitments #58

l-monninger · 2024-05-13T05:00:31Z

Summary

Status: review
RFCs: RFC 29: MCR (draft).
Categories: protocol-units, networks, scripts.

Add MCR commitment processing to Suzuka full node.

Changelog

Suzuka full node settless block commitments with the MCR contract.

Testing

Unit tests of mcr-settlement-manager using the mock of mcr-settlement-client.

Design

The top portion of the graphic above shows a high-level overview of dataflow in the system. Full-nodes execute blocks, generate commitments for the block, and then wait for events from the settlement contract.
The bottom part shows an asynchronous pattern for this scheme wherein a commitment loop is responsible for handling the actual submission via the client to the settlement contract. This may be desirable for a few reasons:
1. Execution can still optimistically proceed, i.e., does not have to wait on a commitment to be accepted.
2. Honest nodes can still avoid making faulty commitments. The job of withholding these commitments is just delegated to the commitment loop.
3. Separation of concern makes it easier to deal with batching and adjustments faulty commitment avoidance scheme (multisig).
4. This can be generalized to a very generic MCR commitment service if the demand presents itself in the SDK.

Entrypoints for understanding

MCR Settlement Client (Stub): https://github.com/movementlabsxyz/movement/tree/eng-326/full-node-commitments/protocol-units/settlement/mcr/client

l-monninger · 2024-05-13T05:01:38Z

We'll use the working CI.

protocol-units/settlement/mcr/client/src/stub.rs

0xmovses

One comment about e2e tests, up to yo whether we want to add them in this PR or a separate one.

util/movement-types/src/lib.rs

l-monninger · 2024-05-13T12:01:33Z

@0xmovses for now, Mikhail will be working against a stub. e2e tests can come elsewhere.

In MonzaExecutor and SuzukaExecutor, return the Commitment value from execute_block. The underlying opt-executor is changed to produce the commitment from proof data.

…full-node-commitments

l-monninger · 2024-05-14T10:45:18Z

@mzabaluev Updated the stub with batching.

l-monninger · 2024-05-14T10:45:43Z

Oh, one more thing you can now expect that will be helpful...

l-monninger · 2024-05-14T10:55:50Z

In the contract and now stub, I added a method get_max_tolerable_height which returns the height to which the contract allows any validator to commit. The corresponding restriction is needed to prevent validators from committing too far ahead and effectively staging an attack that locks down the validator set by assigning every block-height to the current epoch.

Here's the type signature.

async fn get_max_tolerable_block_height(&self) -> Result<u64, anyhow::Error>;

In SuzukaPartialNode, instantiate the stub McrSettlementClient.

Moar data that the executor should be able to immediately provide upon executing the block.

protocol-units/execution/maptos/opt-executor/src/executor.rs

Send the block commitments to the MCR contract via the settlement client. This is done in a separate task from the executor loop to avoid blocking it.

networks/suzuka/suzuka-full-node/src/partial.rs

Add a comment to rename since suzuka needs this Nix-provided stuff as well.

mzabaluev · 2024-05-23T16:49:09Z

networks/suzuka/suzuka-full-node/src/partial.rs

+			let block_metadata = self.executor.build_block_metadata(
+				HashValue::sha3_256_of(block_id.as_bytes()),
+				block_timestamp
+			).await?;
+			let block_metadata_transaction = SignatureVerifiedTransaction::Valid(
+				Transaction::BlockMetadata(
+					block_metadata
+				)
+			);
+			block_transactions.push(block_metadata_transaction);


Aha, I thought this will be somehow pulled through DA, but it needs to be synthesized here.

Maybe even lower it into maptos-opt-executor, then?

Yeah, we may have to reevaluate at some point. But, for now, we are going to assume that the executor can correctly decide the block height based on, well, the number of blocks it has seen.

This is, at least on the surface, a viable assumption because the if it is an honest node, it should fetch all the blocks up from the DA (and when we have state syncing from other nodes that have done so).

However, if we do need have it come up from the DA, the complexity we will have to deal with is that Celestia blob heights are continually pushed regardless of whether additional blobs have been added to a given namespace.

In theory, the sequencer could address by keeping its own counter. But, this could be problematic.

No need to compute the commitment for the metadata workaround block.

l-monninger · 2024-05-23T19:18:39Z

protocol-units/execution/maptos/opt-executor/src/executor.rs

@@ -329,7 +329,7 @@ impl Executor {
 		// Context has a reach-around to the db so the block height should


LGTM hopefully we'll have a productive conversation about where implementations are separate on Friday and Monday.

Scratch that. Build error.

0xmovses

Thanks for this! Just a few thoughts

0xmovses · 2024-05-24T12:11:52Z

flake.nix

+          "--cfg tokio_unstable -C force-frame-pointers=yes -C force-unwind-tables=yes -C link-arg=/STACK:8000000"
+        else
+          "--cfg tokio_unstable -C force-frame-pointers=yes -C force-unwind-tables=yes";
+


👀 Be great if nix did handle config.toml. Seems subpar that it doesn't

0xmovses · 2024-05-24T12:12:39Z

networks/monza/monza-full-node/src/partial.rs

+                    {
+                        tracing::debug!("Got transaction: {:?}", transaction)
+                    }
+


0xmovses · 2024-05-24T12:13:44Z

networks/suzuka/suzuka-full-node/src/partial.rs

-    transaction_sender : Sender<SignedTransaction>,
-    pub transaction_receiver : Receiver<SignedTransaction>,
-    light_node_client: Arc<RwLock<LightNodeServiceClient<tonic::transport::Channel>>>,
+pub struct SuzukaPartialNode<T> {


Let's add in a doc comment for the struct and all its fields.

There should be a wider ticket for documenting our public APIs (when they are stabilized enough to avoid unnecessary churn). I have filed #81 to track this.

0xmovses · 2024-05-24T12:14:45Z

networks/suzuka/suzuka-full-node/src/partial.rs


 	fn bind_transaction_channel(&mut self) {
 		self.executor.set_tx_channel(self.transaction_sender.clone());
 	}

-	pub fn bound(
+	pub fn bound<C>(


any pub methods should have a doc comment.

0xmovses · 2024-05-24T12:15:53Z

networks/suzuka/suzuka-full-node/src/partial.rs

@@ -110,9 +140,9 @@ impl <T : SuzukaExecutor + Send + Sync + Clone>SuzukaPartialNode<T> {
    // receive transactions from the transaction channel and send them to be executed
    // ! This assumes the m1 da light node is running sequencer mode
    pub async fn read_blocks_from_da(&self) -> Result<(), anyhow::Error> {
-        
+


Let's turn those comments into doc comments

0xmovses · 2024-05-24T12:22:59Z

protocol-units/execution/suzuka/executor/src/v1.rs

-        executor.execute_block(FinalityMode::Opt, block).await?;
+        let commitment = executor.execute_block(FinalityMode::Opt, block).await?;
+
+        // TODO: test the commitment


Is this TODO outside the scope of this PR? Maybe lets make a tracking ticket . issue.

I've added some checks in fae30be
Not testing the commitment hash itself, it would require replicating much of the internal code.

protocol-units/settlement/mcr/contracts/broadcast/DeployMCR.s.sol/1569/run-1716348078.json

protocol-units/settlement/mcr/contracts/broadcast/DeployMCR.s.sol/1569/run-latest.json

protocol-units/settlement/mcr/manager/src/manager.rs

Don't use the "logging" feature to gate individual tracing macro expansions, tracing is designed to not need this. Remove dependencies on env_logger and tracing-log, these crates are no longer in use.

Test some properties of the BlockCommitment returned by a test from the execute_block method. There's no easy way to verify the commitment hash itself.

Rename to override_block_commitment for clarity.

To test dynamic behavior of the McrSettlementManager, add methods to pause and resume streaming of commitments.

Test dynamic behavior: when the client is not ready to process more incoming blocks, the manager should pause with blocks as well. Then when the client is ready to accept more, resume.

Included Liam's remarks on treating verifier errors as successful verification.

andyjsbell

lgtm

fix: adding stub.

e61c112

l-monninger marked this pull request as draft May 13, 2024 05:00

Merge branch 'l-monninger/ci-fix' into eng-326/full-node-commitments

8228d24

mzabaluev reviewed May 13, 2024

View reviewed changes

protocol-units/settlement/mcr/client/src/stub.rs Outdated Show resolved Hide resolved

l-monninger added 2 commits May 13, 2024 04:48

fix: remove duplicate trait.

bd6c85d

fix: cleanup.

15c7781

0xmovses reviewed May 13, 2024

View reviewed changes

util/movement-types/src/lib.rs Show resolved Hide resolved

mcr-settlement-client: fmt

e4a5b65

mzabaluev and others added 8 commits May 13, 2024 18:08

Merge branch 'main' into eng-326/full-node-commitments

c225540

movement-types: Commitment from StateProof

eee8edd

opt-executor: reorder use statements

edc750f

Return Commitment from executors' execute_block

1e5d803

In MonzaExecutor and SuzukaExecutor, return the Commitment value from execute_block. The underlying opt-executor is changed to produce the commitment from proof data.

fix: drop contracts.

4c38cc2

Merge branch '0xmovses/ENG-317-implement-mcr-contracts' into eng-326/…

04972b7

…full-node-commitments

fix: swap tonic for async trait.

b1a41ba

fix: merge.

1f6792d

feat: add max tolerable height.

959fbbb

mzabaluev mentioned this pull request May 14, 2024

Executor API cleanup and simplification #61

Merged

mzabaluev added 3 commits May 14, 2024 15:29

feat: Instantiate MCR settlement client

2753f1d

In SuzukaPartialNode, instantiate the stub McrSettlementClient.

Merge branch 'main' into eng-326/full-node-commitments

6c7670c

Return BlockCommitment from execute_block

01cd707

Moar data that the executor should be able to immediately provide upon executing the block.

mzabaluev reviewed May 14, 2024

View reviewed changes

protocol-units/execution/maptos/opt-executor/src/executor.rs Outdated Show resolved Hide resolved

feat(suzuka-full-node): settle block commitments

aad48b8

Send the block commitments to the MCR contract via the settlement client. This is done in a separate task from the executor loop to avoid blocking it.

mzabaluev reviewed May 15, 2024

View reviewed changes

networks/suzuka/suzuka-full-node/src/partial.rs Outdated Show resolved Hide resolved

print error returned by process_commitment

de96f0d

l-monninger and others added 11 commits May 22, 2024 04:56

fix: prebuild.

03e626e

Fix up naming in suzuka scripts again

489d7c8

CI: disable MOVEMENT_PREBUILT

bdc0388

CI: don't run prebuild script

8a332be

scripts: fix up m1-da-light-node build

76fce46

scripts: restore monza-aptos naming

2719489

Add a comment to rename since suzuka needs this Nix-provided stuff as well.

CI: print available filesystem space

9ca5b08

fix: movement-runner down to debug.

e8d9dca

fix: bad prebuild of aptos-faucet-service.

a0c7681

fix: drop containers from ci for now.

abd1bb7

fix: missing block metadata.

2c2a2ec

mzabaluev reviewed May 23, 2024

View reviewed changes

l-monninger removed the request for review from andyjsbell May 23, 2024 17:45

mzabaluev added 2 commits May 23, 2024 21:51

opt-executor: refactor execute_block_inner

6dc612f

No need to compute the commitment for the metadata workaround block.

Implement get_block_head_height in executors

146c51d

l-monninger commented May 23, 2024

View reviewed changes

opt-executor: fix up compile

f0abc03

0xmovses reviewed May 24, 2024

View reviewed changes

mzabaluev added 6 commits May 24, 2024 18:32

streamline logging

47e90a5

Don't use the "logging" feature to gate individual tracing macro expansions, tracing is designed to not need this. Remove dependencies on env_logger and tracing-log, these crates are no longer in use.

chore: remove and ignore MCR contract logs

06ce28d

suzuka-executor: sanity-test block commitment

fae30be

Test some properties of the BlockCommitment returned by a test from the execute_block method. There's no easy way to verify the commitment hash itself.

mcr-settlement-client: rename settle method

63726a0

Rename to override_block_commitment for clarity.

mcr-settlement-client: pause and resume for mock

b4999b3

To test dynamic behavior of the McrSettlementManager, add methods to pause and resume streaming of commitments.

mcr-settlement-manager: test back pressure

df9cb38

Test dynamic behavior: when the client is not ready to process more incoming blocks, the manager should pause with blocks as well. Then when the client is ready to accept more, resume.

mzabaluev force-pushed the eng-326/full-node-commitments branch from a76e9ed to df9cb38 Compare May 27, 2024 14:04

mzabaluev mentioned this pull request May 27, 2024

ENG-400: Generalized protocol-unit-types, error handling upgrade and async-trait refactoring #71

Merged

m1-da-light-node: expanded FIXME comment

e5feea0

Included Liam's remarks on treating verifier errors as successful verification.

andyjsbell approved these changes May 28, 2024

View reviewed changes

andyjsbell merged commit 3dadd48 into main May 28, 2024
3 checks passed

l-monninger mentioned this pull request May 29, 2024

Errors running monza-full-node and suzuka-full-node #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-326: Full Node Commitments #58

ENG-326: Full Node Commitments #58

l-monninger commented May 13, 2024 •

edited by mzabaluev

Loading

l-monninger commented May 13, 2024

0xmovses left a comment

l-monninger commented May 13, 2024

l-monninger commented May 14, 2024 •

edited

Loading

l-monninger commented May 14, 2024

l-monninger commented May 14, 2024 •

edited

Loading

mzabaluev May 23, 2024

mzabaluev May 23, 2024

l-monninger May 23, 2024

l-monninger May 23, 2024

l-monninger May 23, 2024

l-monninger May 23, 2024 •

edited

Loading

0xmovses left a comment

0xmovses May 24, 2024

0xmovses May 24, 2024

0xmovses May 24, 2024

mzabaluev May 27, 2024

0xmovses May 24, 2024

0xmovses May 24, 2024

0xmovses May 24, 2024

mzabaluev May 27, 2024

andyjsbell left a comment

		@@ -329,7 +329,7 @@ impl Executor {
		// Context has a reach-around to the db so the block height should

ENG-326: Full Node Commitments #58

ENG-326: Full Node Commitments #58

Conversation

l-monninger commented May 13, 2024 • edited by mzabaluev Loading

Summary

Changelog

Testing

Design

Entrypoints for understanding

l-monninger commented May 13, 2024

0xmovses left a comment

Choose a reason for hiding this comment

l-monninger commented May 13, 2024

l-monninger commented May 14, 2024 • edited Loading

l-monninger commented May 14, 2024

l-monninger commented May 14, 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

l-monninger May 23, 2024 • edited Loading

Choose a reason for hiding this comment

0xmovses 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

andyjsbell left a comment

Choose a reason for hiding this comment

l-monninger commented May 13, 2024 •

edited by mzabaluev

Loading

l-monninger commented May 14, 2024 •

edited

Loading

l-monninger commented May 14, 2024 •

edited

Loading

l-monninger May 23, 2024 •

edited

Loading