block-version-based protocol evolution #1468

cbeck88 · 2022-02-09T17:58:53Z

This makes transaction builder and validation aware of BlockVersion.
It also plumbs this config into consensus.

This commit adds support for the change in fog sample paykit
and mobilecoind

Checklist:

Rebase on master
Do the responder id parts
I made the block version be a parameter to things like form_block, but it might be better to do it differently, e.g. block version is announced on enclave startup, and can change after form_block but not in between calls to form block?
There is a place where I am taking a max of latest_block.version and configured block version, but perhaps it should only ever use the configured block version.
In many tests, I decided it was worth testing all acceptable values of block version, and in other places, I only tested BlockVersion(1) or BlockVersion::MAX because that seemed like the highest value testing. Review these decisions.

cbeck88 · 2022-02-09T18:44:35Z

transaction/core/src/validation/validate.rs

+        validate_memos_exist(tx)?;
+    } else {
+        validate_no_memos_exist(tx)?;
+    }



This is one of the main points of the PR

cbeck88 · 2022-02-09T18:46:14Z

transaction/std/src/transaction_builder.rs

@@ -65,10 +67,11 @@ impl<FPR: FogPubkeyResolver> TransactionBuilder<FPR> {
    /// * `memo_builder` - An object which creates memos for the TxOuts in this
    ///   transaction
    pub fn new<MB: MemoBuilder + 'static + Send + Sync>(
+        block_version: BlockVersion,


This is one of the other main points of the PR

cbeck88 · 2022-02-09T18:51:13Z

transaction/std/src/memo_builder/mod.rs

@@ -34,15 +38,15 @@ pub trait MemoBuilder: Debug {
        value: u64,
        recipient: &PublicAddress,
        memo_context: MemoContext,
-    ) -> Result<Option<MemoPayload>, NewMemoError>;
+    ) -> Result<MemoPayload, NewMemoError>;


I changed this because in the new world it doesn't make sense for a memo builder to sometimes omit the memo and sometimes not -- the block version controls if the memo is present or not, and the memo builder can just write MemoPayload::default if they don't want to write any content. This also means NoMemoBuildergoes away and is replaced by EmptyMemoBuilder everywhere it was used, since it will do the right thing based on Block version.

cbeck88 · 2022-02-09T18:52:40Z

transaction/std/src/transaction_builder.rs

        let result = self.add_output_with_fog_hint_address(
            value,
            recipient,
            recipient,
-            |memo_ctxt| mb.make_memo_for_output(value, recipient, memo_ctxt),
+            |memo_ctxt| {
+                if block_version.e_memo_feature_is_supported() {


This is where the transaction builder uses block version to decide whether or not to filter out memos

jcape

Comments on BlockVersion (namely, keep it private, and use try_from to check whether a version is supported -- this is where the "this version is not supported" check happens, rather than making everyone that's consuming a chain implement their own version).

transaction/core/src/blockchain/block_version.rs

eranrund · 2022-02-09T22:55:36Z

Tip for reviewers: consider pulling locally and doing git diff --ignore-space-change master. A lot of the changes here are indentation and result in an enormous diff in GitHub.

remoun · 2022-02-09T23:16:38Z

Tip for reviewers: consider pulling locally and doing git diff --ignore-space-change master. A lot of the changes here are indentation and result in an enormous diff in GitHub.

Agreed. Fortunately GH also supports ignoring whitespace with ?w=1: https://github.com/mobilecoinfoundation/mobilecoin/pull/1468/files?diff=split&w=1

eranrund · 2022-02-09T23:26:54Z

Tip for reviewers: consider pulling locally and doing git diff --ignore-space-change master. A lot of the changes here are indentation and result in an enormous diff in GitHub.

Agreed. Fortunately GH also supports ignoring whitespace with ?w=1: https://github.com/mobilecoinfoundation/mobilecoin/pull/1468/files?diff=split&w=1

TIL! Thanks!

fog/sample-paykit/src/cached_tx_data/mod.rs

ledger/db/src/lib.rs

cbeck88 · 2022-02-16T17:51:44Z

transaction/core/src/blockchain/block.rs

@@ -122,7 +123,7 @@ impl Block {
    /// * `root_element` - The root element for membership proofs
    /// * `block_contents` - Contents of the block.
    pub fn new(
-        version: u32,
+        version: BlockVersion,


we could consider that Block doesn't use a BlockVersion and uses a u32 instead, since it is using u32 internally. that would make the ledger db test around adding too recent blocks still possible.

I personally prefer this version with the transmute hack in the test.

+1 for keeping BlockVersion and using the hack in test

cbeck88 · 2022-02-16T17:52:52Z

transaction/core/src/validation/validate.rs

        let mut ledger = create_ledger();
        let n_blocks = 1;
-        initialize_ledger(&mut ledger, n_blocks, &sender, &mut rng);
+        initialize_ledger(
+            adapt_hack(&block_version),


I would really like to refactor to kill off this adapt_hack stuff, I think we just have to reorganize the crates a little bit.

That would be nice (in a followup PR)

cbeck88 · 2022-02-16T17:58:18Z

I think this may be ready to go, I fixed up all the consensus-service plumbing:

enclave_init takes the BlockVersion and it is not an argument to form_block or the tx validation routine, instead it is stored in the enclave and can't be changed after init.
That's better because the BlockVersion needs to go into the responder id, and the idea right now is that the network needs to be restarted to change the BlockVersion. (We can investigate protocol version scp statements in a later PR)
BlockVersion now has strong invariants and uses #[serde(try_from = "u32")] to prevent skirting them
Most of the changes are now in tests. There is some question if Block::new( should actually take a BlockVersion.

cbeck88 · 2022-02-16T18:10:10Z

ledger/db/src/lib.rs

            let invalid_block = Block::new_with_parent(
-                last_block.version + 1,
+                unsafe { core::mem::transmute(last_block.version + 1) },


let me know if you guys have feelings about this, i think this is a valuable test to keep running, and this lets us bypass the invariant enforced by BlockVersion that the version can't exceed BlockVersion::MAX

i guess another option is to make add_block function of ledger db not take a BlockVersion, and just take a u32 and internally convert it to a BlockVersion

I think its better to keep using the proper type and doing this hack in the test code.

consensus/enclave/impl/src/lib.rs

transaction/core/src/validation/validate.rs

consensus/enclave/impl/src/lib.rs

consensus/service/src/bin/main.rs

…sion formatting

consensus/enclave/impl/src/lib.rs

crypto/digestible/src/lib.rs

transaction/core/src/blockchain/block_version.rs

fog/sample-paykit/src/cached_tx_data/mod.rs

Co-authored-by: Remoun Metyas <[email protected]>

cbeck88 · 2022-02-18T21:29:34Z

Here's the comparison with deployment of current master to the same network

cbeck88 · 2022-02-18T21:31:08Z

it looks maybe a little worse to me towards the end of the slam, but peak is still ~40 tps, not sure if the rest of the graph is statistically significant

cbeck88 · 2022-02-18T21:32:02Z

i'm going to try deploying this PR a second time

eranrund

LGTM. I think it's okay to merge even if performance is a bit degraded because this is an important piece in many upcoming changes and releases. It would be great if we could find the time to do some profiling of stuff in the enclave and consensus-service. It's been awhile since we went through that exercise..

cbeck88 · 2022-02-18T23:06:33Z

Here are the results from the second deploy (of this PR):

eranrund · 2022-02-18T23:26:23Z

20s tx completion time is a pretty big difference than 15s I am used to seeing...

cbeck88 · 2022-02-18T23:35:58Z

@eranrund yeah i'm not sure what could be causing this, maybe its because we are holding the mutex for the fees for a longer period of time?

cbeck88 · 2022-02-18T23:37:05Z

maybe we should try replacing the Mutex<Option<...>> for the config with a OnceCell

eranrund · 2022-02-18T23:56:28Z

@eranrund yeah i'm not sure what could be causing this, maybe its because we are holding the mutex for the fees for a longer period of time?

Interesting, maybe?
You could try moving this https://github.com/garbageslam/mobilecoin/blob/block-version-aware-tx-builder-validation/consensus/enclave/impl/src/lib.rs#L367-L371 into a scoped block and cloning the stuff you need outside of the scope (just the minimum fee and block version)
Should be an easy test and seems like thats a good change regardless.

cbeck88 · 2022-02-19T00:18:04Z

I just made a quick patch like this:

cbeck88#4

and pushed it to build, let's see what happens

(I didn't see your suggestion before I started working on it this way)

cbeck88 · 2022-02-19T00:55:00Z

That makes the TPS look better, the completion time is still not 15 though... but better than before

jcape

This generally looks good to me, the only real question is what BlockVersion::default() should actually default to---it feels to me like it should be ONE (or MIN, or whatever), not MAX, but I'm also not sure where it's actually used...

jcape · 2022-02-18T01:18:00Z

consensus/enclave/api/src/fee_map.rs

 use mc_transaction_core::{tokens::Mob, Token, TokenId};
 use serde::{Deserialize, Serialize};

 /// A thread-safe object that contains a map of fee value by token id.
-#[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq)]
+#[derive(Clone, Debug, Deserialize, Digestible, Eq, Hash, PartialEq, Serialize)]
 pub struct FeeMap {


Can this get reduced to a newtype struct?

Probably but lets do it in a subsequent PR, it's kind of orthogonal to this PR, and there's a lot more token config stuff coming down the pipeline

jcape · 2022-02-18T23:33:41Z

consensus/enclave/api/src/config.rs

+    fn default() -> Self {
+        Self {
+            fee_map: FeeMap::default(),
+            block_version: BlockVersion::MAX,


I think we'd want this to be ::MIN, right?

Remoun had suggested ::MAX in earlier review. (IIRC)
I think the default doesn't actually matter here, the actual default in the enclave is None basically, and the struct_opt default I think is 1 for backwards compat

consensus/enclave/api/src/error.rs

consensus/enclave/impl/src/lib.rs

jcape · 2022-02-19T01:45:53Z

consensus/enclave/impl/src/lib.rs

@@ -564,7 +599,7 @@ impl ConsensusEnclave for SgxConsensusEnclave {

        // Form the block.
        let block = Block::new_with_parent(


If we want to do this, can we make a ticket for it?

consensus/enclave/impl/src/lib.rs

jcape · 2022-02-19T02:27:38Z

ledger/db/src/lib.rs

    use mc_util_from_random::FromRandom;
    use rand::{rngs::StdRng, SeedableRng};
    use rand_core::RngCore;
    use tempdir::TempDir;
    use test::Bencher;

+    // TODO: Should these tests run over several block versions?
+    const BLOCK_VERSION: BlockVersion = BlockVersion::ONE;


Is this used? The tests below appear to all use BlockVersion::ONE directly...

In my search I see a lot of uses of BLOCK_VERSION in this file and no uses of BlockVersion::ONE. If you prefer we can mass replace them

add once-cell for config in block-version-aware PR

cbeck88 · 2022-02-19T19:07:32Z

I ran it through deploy again -- I think it's possible that when I tested the once-cell change i didn't deploy the commit I thought I was deploying. These numbers look better (after merging the once-cell part to the PR)

cbeck88 requested review from jcape, remoun, mfaulk, eranrund, sugargoat and a team February 9, 2022 17:58

cbeck88 commented Feb 9, 2022

View reviewed changes

jcape reviewed Feb 9, 2022

View reviewed changes

jcape assigned cbeck88 Feb 14, 2022

cbeck88 force-pushed the block-version-aware-tx-builder-validation branch 2 times, most recently from 2e3277b to 86c37a3 Compare February 16, 2022 16:40

cbeck88 commented Feb 16, 2022

View reviewed changes

fog/sample-paykit/src/cached_tx_data/mod.rs Outdated Show resolved Hide resolved

cbeck88 commented Feb 16, 2022

View reviewed changes

ledger/db/src/lib.rs Show resolved Hide resolved

cbeck88 commented Feb 16, 2022

View reviewed changes

cbeck88 changed the title ~~proof of concept for block-version-based protocol evolution~~ block-version-based protocol evolution Feb 16, 2022

cbeck88 commented Feb 16, 2022

View reviewed changes

consensus/enclave/impl/src/lib.rs Outdated Show resolved Hide resolved

cbeck88 commented Feb 16, 2022

View reviewed changes

transaction/core/src/validation/validate.rs Show resolved Hide resolved

eranrund reviewed Feb 16, 2022

View reviewed changes

consensus/enclave/impl/src/lib.rs Outdated Show resolved Hide resolved

eranrund reviewed Feb 16, 2022

View reviewed changes

consensus/service/src/bin/main.rs Outdated Show resolved Hide resolved

make form block reject decreasing block version, fix bug in block ver…

f55fc2d

…sion formatting

cbeck88 requested a review from eranrund February 18, 2022 20:03

sugargoat added the multi-asset label Feb 18, 2022

jcape removed the multi-asset label Feb 18, 2022

remoun approved these changes Feb 18, 2022

View reviewed changes

Chris Beck and others added 2 commits February 18, 2022 13:57

Update crypto/digestible/src/lib.rs

35e4237

Co-authored-by: Remoun Metyas <[email protected]>

fix comments in digestible crate

974ff80

cbeck88 modified the milestones: (2022-Q1) Multiple Token Types, Blockchain Format Evolution Feb 18, 2022

cbeck88 added 2 commits February 18, 2022 14:19

fix nits

d2a58f6

add an error type for enclave not initialized

aed2e61

eranrund approved these changes Feb 18, 2022

View reviewed changes

add once-cell for config in block-version-aware PR

00c8aab

jcape approved these changes Feb 19, 2022

View reviewed changes

cbeck88 and others added 2 commits February 19, 2022 11:23

resolve jcape comments

0af895f

Merge pull request #4 from garbageslam/block-version-aware-with-oncecell

191020e

add once-cell for config in block-version-aware PR

cbeck88 merged commit b3e6620 into mobilecoinfoundation:master Feb 19, 2022

		@@ -564,7 +599,7 @@ impl ConsensusEnclave for SgxConsensusEnclave {

		// Form the block.
		let block = Block::new_with_parent(

block-version-based protocol evolution #1468

block-version-based protocol evolution #1468

Conversation

cbeck88 commented Feb 9, 2022 • 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

jcape left a comment

Choose a reason for hiding this comment

eranrund commented Feb 9, 2022

remoun commented Feb 9, 2022

eranrund commented Feb 9, 2022

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

cbeck88 commented Feb 16, 2022

cbeck88 Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

cbeck88 Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeck88 commented Feb 18, 2022

cbeck88 commented Feb 18, 2022

cbeck88 commented Feb 18, 2022

eranrund left a comment

Choose a reason for hiding this comment

cbeck88 commented Feb 18, 2022

eranrund commented Feb 18, 2022

cbeck88 commented Feb 18, 2022

cbeck88 commented Feb 18, 2022 • edited Loading

eranrund commented Feb 18, 2022

cbeck88 commented Feb 19, 2022 • edited Loading

cbeck88 commented Feb 19, 2022 • edited Loading

jcape 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

cbeck88 Feb 19, 2022 • 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

cbeck88 commented Feb 19, 2022

cbeck88 commented Feb 9, 2022 •

edited

Loading

cbeck88 Feb 16, 2022 •

edited

Loading

cbeck88 Feb 16, 2022 •

edited

Loading

cbeck88 commented Feb 18, 2022 •

edited

Loading

cbeck88 commented Feb 19, 2022 •

edited

Loading

cbeck88 commented Feb 19, 2022 •

edited

Loading

cbeck88 Feb 19, 2022 •

edited

Loading