[types] add ChainID to user txn #4969

christinacelee · 2020-07-08T23:27:33Z

Motivation

Adding ChainId to a user txn, to prevent txns potentially being replayed across networks (chains). The ChainID is represented as a u8

In addition to the regular ChainId, this PR also adds the ReservedChain type to specify chain IDs that are reserved. This enum type is primarily used to improve human readability of chain ID in config files and CLI (is used as an adaptor from string to the actual u8 representation of the chain ID).

Test plan

Checked that CLI and config can consume both human-readable and numeric input of chain ID

christinacelee · 2020-07-09T00:54:37Z

some of the changes are uncharted territory for me personally, especially KeyManager/secure/config things, so please take a look and make sure I'm not doing something obviously clowny! @JoshLind

gregnazario

lgtm, let's have someone else look over it

gregnazario · 2020-07-09T00:54:02Z

types/src/chain_id.rs

+
+impl Display for ChainId {
+    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
+        write!(f, "ChainId {}", self.0)


Should we just print the ChainId number without the word ChainId?

I thought ChainId might make things easier to debug (having just the u64 might be unhelpful?), others can weigh in too

if it's an enum we'll get the chainId name directly :)

actually I'm thinking that we might want to go further, it'd be good if we can just have "mainnet" or "testnet" as chainid in CLIs or config files : o wdyt? (it sounds like having a number would make reading config files harder)

gregnazario · 2020-07-09T00:55:20Z

types/src/chain_id.rs

+impl Default for ChainId {
+    fn default() -> Self {
+        ChainId::new(1)
+    }
+}


If the MOCK_CHAIN_ID and the default are going to be the same ID, should we just use the constant for both?

+1 (I'm a big fan of just using constants where applicable to remove ambiguity about what ID is actually being picked)

gregnazario · 2020-07-09T00:58:44Z

types/src/chain_id.rs

+};
+
+#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct ChainId(pub u64);


I think it will still work without the u64 as pub

enum is better than a u64 imo

agree, I think we should use an enum here, similar to what we did for NetworkId IIRC

gregnazario · 2020-07-09T01:02:08Z

network/src/transport.rs

-            chain_id.clone(),
+            chain_id,


A nice side effect of it now being a number.

gregnazario · 2020-07-09T01:07:20Z

config/src/config/key_manager_config.rs

@@ -32,6 +34,7 @@ impl Default for KeyManagerConfig {
            secure_backend: SecureBackend::InMemoryStorage,
            sleep_period_secs: DEFAULT_SLEEP_PERIOD_SECS,
            txn_expiration_secs: DEFAULT_TXN_EXPIRATION_SECS,
+            chain_id: ChainId::mainnet(),


I'd be concerned with a default that connects to the mainnet chain. I'll let @JoshLind make the call on that one though.

I don't have strong feelings, but I think this should probably match whatever default we're using in the actual validator/node configs.

zekun000

not for this pr, but it's worth discussing whether we want to read the chain id from genesis upon start to eliminate two source of truth (config + genesis)

sblackshear · 2020-07-09T02:44:28Z

@zekun000 I think we should definitely read it from genesis, otherwise validators with different configs will disagree...

zekun000 · 2020-07-09T02:58:10Z

sounds like a good on chain config value :)

mimoo · 2020-07-09T03:07:55Z

types/src/chain_id.rs

+
+    #[cfg(any(test, feature = "fuzzing"))]
+    pub fn mock() -> Self {
+        ChainId::new(MOCK_CHAIN_ID)


I feel like we tend to use "mock" for components, and "testing" for values

mimoo · 2020-07-09T03:13:19Z

Good job on the PR! This looks good to me (besides the ChainId u64 -> Enum), as this is touching a lot of stuff I feel like someone else than me should approve this :D

not for this pr, but it's worth discussing whether we want to read the chain id from genesis upon start to eliminate two source of truth (config + genesis)

I would prefer that we have it in both the config AND the genesis, this way if my config says "testnet" but I start with the "mainnet" genesis then it won't work. A good way to avoid configuration mistakes and be explicit about what we're doing.

sblackshear · 2020-07-09T03:53:14Z

Good job on the PR! This looks good to me (besides the ChainId u64 -> Enum), as this is touching a lot of stuff I feel like someone else than me should approve this :D

I also prefer enums to plain numbers in general, but Move does not have enums... So I would vote for sticking with the u64 to avoid Rust -> Move conversion surprises.

I would prefer that we have it in both the config AND the genesis, this way if my config says "testnet" but I start with the "mainnet" genesis then it won't work. A good way to avoid configuration mistakes and be explicit about what we're doing.

Makes sense, but at what point in the code should we compare genesis to the config and fail?

davidiw · 2020-07-09T05:39:03Z

Thanks for pushing this PR out! It has been a loooong time coming :D!!!!

How did we land on using a u64 to represent the ChainId versus the string earlier? My guess is so that it could be incorporated into the transaction at low cost?

Couple of discussion thoughts / topics:

How do we distinguish against the internal / external / third-party chains?
How do we ensure that admins properly update this value in their config?

One idea we had floating around was to make the ChainId deterministic computation of shared data plus some fixed shared counter. E.g., "ledger version:ledger hash:counter", but that would be a lot of fields and in a string, so not that great... Have we considered a hash?

Also one concern about this being within an "on-chain config" is -- would that make it mutable? Is that a property we want to have? Is it possible to have immutable values in on-chain configs?

sblackshear · 2020-07-09T17:09:06Z

Also one concern about this being within an "on-chain config" is -- would that make it mutable? Is that a property we want to have? Is it possible to have immutable values in on-chain configs?

Agreed that we probably don't want this to be an on-chain config because it's immutable. It should just be an ordinary constant written at genesis.

zekun000 · 2020-07-09T17:12:46Z

I'm a little confused about immutability, if the mainnet forks do we want to change the chain id e.g. a fork enabling more risk smart contracts? from what I heard from @aching we do, or we simply change it by writeset.

aching · 2020-07-09T17:44:14Z

I think @sblackshear is right (immutable). Supporting easy forks doesn't seem to be a use case. Writeset seems reasonable to me.

aching · 2020-07-09T17:50:22Z

How did we land on using a u64 to represent the ChainId versus the string earlier? My guess is so that it could be incorporated into the transaction at low cost?

u64, u32, enum, etc. become a single byte if mainnet is 0.

Couple of discussion thoughts / topics:

How do we distinguish against the internal / external / third-party chains?

I think the only idea is LA will define a set of chains (i.e. mainnet = 0). Any other chain can pick any id. Perhaps if we want LA could also define ranges for private chains, testnet chains, etc.

How do we ensure that admins properly update this value in their config?

One idea we had floating around was to make the ChainId deterministic computation of shared data plus some fixed shared counter. E.g., "ledger version:ledger hash:counter", but that would be a lot of fields and in a string, so not that great... Have we considered a hash?

This is about a tradeoff of debuggability of safety vs size. Hash is bigger than a byte. =)

Also one concern about this being within an "on-chain config" is -- would that make it mutable? Is that a property we want to have? Is it possible to have immutable values in on-chain configs?

Agree on immutable.

xli · 2020-07-09T18:05:01Z

types/src/chain_id.rs

+
+const MAINNET_CHAIN_ID: u64 = 0;
+#[cfg(any(test, feature = "fuzzing"))]
+pub const MOCK_CHAIN_ID: u64 = 1;


should we just have TESTNET_CHAIN_ID here and for all tests? mock gives impression it is generated number.
also, since 0 is mainnet, 1 wil be testnet?

JoshLind · 2020-07-09T18:44:25Z

config/management/src/validator_config.rs

@@ -33,6 +34,8 @@ pub struct ValidatorConfig {
    fullnode_address: NetworkAddress,
    #[structopt(flatten)]
    backends: SecureBackends,
+    #[structopt(long)]
+    chain_id: ChainId,


I don't think this is quite right, as it's essentially setting a chainID on a single transaction used during the genesis process (this transaction is submitted by each validator operator). Instead, my feeling is that we'll probably want to have the association determine the ID and set it in genesis once, before bootstrapping the network. However, from reading the comments it seems like there's still a lot of on-going discussions about how this should actually work..

(If this is something that the association is going to decide at genesis, we may want to setup a sync with @davidiw, to figure out the best place to do this for the management tool)

bors-libra · 2020-07-10T20:07:15Z

☔ The latest upstream changes (presumably 2f6a802) made this pull request unmergeable. Please resolve the merge conflicts.

bors-libra · 2020-07-13T13:26:36Z

☔ The latest upstream changes (presumably 0f911dd) made this pull request unmergeable. Please resolve the merge conflicts.

thefallentree · 2020-07-13T22:06:11Z

please also add changes to json-rpc/types/view.rs to show chainID in JSONRPC

thefallentree · 2020-07-13T22:06:35Z

as well as updating "json-rpc-spec.md"

thefallentree · 2020-07-14T01:23:04Z

LGTM on jsonrpc side.

aching

This is awesome. Looks good to me, I have a few minor comments.

@bors-libra delegate+

aching · 2020-07-14T01:46:52Z

client/swiss-knife/src/main.rs

@@ -117,6 +121,8 @@ struct TxnParams {
    pub sender_address: String,
    // Sequence number of this transaction corresponding to sender's account.
    pub sequence_number: u64,
+    // Chain ID of the network this transaction is intended for


Suggested change

// Chain ID of the network this transaction is intended for

// Chain ID of the Libra network this transaction is intended for

aching · 2020-07-14T01:52:58Z

json-rpc/json-rpc-spec.md

+   </td>
+   <td>u8
+   </td>
+   <td>Chain ID of the network this transaction is intended for


Suggested change

<td>Chain ID of the network this transaction is intended for

<td>Chain ID of the Libra network this transaction is intended for

aching · 2020-07-14T01:55:21Z

testsuite/cli/src/client_proxy.rs

@@ -102,6 +103,8 @@ pub struct IndexAndSequence {

 /// Proxy handling CLI commands/inputs.
 pub struct ClientProxy {
+    /// chain ID of the network this client is interacting with


Suggested change

/// chain ID of the network this client is interacting with

/// chain ID of the Libra network this client is interacting with

aching · 2020-07-14T01:56:22Z

testsuite/cli/src/main.rs

+        help = "\
+            Explicitly specify the chain ID of the network the CLI is connecting to: e.g.,
+            for mainnet: \"MAINNET\" or 0, pre-mainnet: \"PREMAINNET\" or 1,
+            testnet: \"TESTNET\" or 2, devnet: \"DEVNET\" or 3
+        "


I feel like we might want to print this from the 'ReservedChain' values to ensure it doesn't get out of sync.

aching · 2020-07-14T02:38:06Z

types/src/chain_id.rs

+pub enum ReservedChain {
+    MAINNET,
+    PREMAINNET,
+    TESTNET,
+    DEVNET,
+    TESTING,
+}


When it comes to reserved Libra networks, I agree MAINNET = 0.

However, PREMAINNET, TESTNET, DEVNET, TESTING, are non-reserved in my opinion. They could evolve over time. One thing we could do if we want to keep a single enum is something like below - renaming to NamedChain and then clearly laying out what is reserved and what is not:

Suggested change

pub enum ReservedChain {

MAINNET,

PREMAINNET,

TESTNET,

DEVNET,

TESTING,

}

pub enum NamedChain {

/// MAINNET is the Libra mainnet production chain and is reserved for 0.

MAINNET,

// The Libra chain below are non-reserved, non-production, and may change over time. They are listed for convenience here.

PREMAINNET,

TESTNET,

DEVNET,

TESTING,

}

aching · 2020-07-14T02:39:29Z

types/src/transaction/mod.rs

@@ -80,6 +81,9 @@ pub struct RawTransaction {
    #[serde(serialize_with = "serialize_duration")]
    #[serde(deserialize_with = "deserialize_duration")]
    expiration_time: Duration,
+
+    // chain ID of the network this transaction is intended for


Suggested change

// chain ID of the network this transaction is intended for

// chain ID of the Libra network this transaction is intended for

christinacelee · 2020-07-15T00:13:40Z

@bors-libra r=aching

bors-libra · 2020-07-15T00:13:41Z

📌 Commit 76a7c36 has been approved by aching

bors-libra · 2020-07-15T00:20:20Z

⌛ Testing commit 76a7c36 with merge fbd7352...

Closes: #4969 Approved by: aching

github-actions · 2020-07-15T00:36:52Z

Cluster Test Result

all up : 1040 TPS, 4352 ms latency, 5500 ms p99 latency, no expired txns

Repro cmd:

./scripts/cti --tag land_fbd73524 --run bench

bors-libra · 2020-07-15T00:39:12Z

☀️ Test successful - checks-actions_land_blocking_test, checks-circle_commit_workflow
Approved by: aching
Pushing fbd7352 to master...

christinacelee · 2020-07-17T20:00:27Z

config/src/config/key_manager_config.rs

@@ -21,6 +22,8 @@ pub struct KeyManagerConfig {
    pub secure_backend: SecureBackend,
    pub sleep_period_secs: u64,
    pub txn_expiration_secs: u64,
+    #[serde(deserialize_with = "chain_id::deserialize_config_chain_id")]
+    pub chain_id: ChainId,


chain ID needs to be added to key manager config too @mgorven @sausagee

@runtian-zhou

christinacelee force-pushed the txn-chain-id branch 2 times, most recently from 01bd851 to 23d5442 Compare July 9, 2020 00:47

christinacelee requested review from gregnazario, JoshLind, mimoo, xli and davidiw July 9, 2020 00:49

christinacelee requested a review from zekun000 July 9, 2020 00:57

gregnazario reviewed Jul 9, 2020

View reviewed changes

christinacelee force-pushed the txn-chain-id branch from 23d5442 to e1ad7d6 Compare July 9, 2020 01:12

zekun000 reviewed Jul 9, 2020

View reviewed changes

christinacelee force-pushed the txn-chain-id branch from e1ad7d6 to 4a08edd Compare July 9, 2020 01:42

mimoo reviewed Jul 9, 2020

View reviewed changes

moeziniaf self-requested a review July 9, 2020 11:18

aching self-requested a review July 9, 2020 17:42

xli reviewed Jul 9, 2020

View reviewed changes

JoshLind reviewed Jul 9, 2020

View reviewed changes

christinacelee force-pushed the txn-chain-id branch from 4a08edd to be79449 Compare July 9, 2020 19:28

christinacelee force-pushed the txn-chain-id branch from be79449 to 8cb5e56 Compare July 11, 2020 05:27

[network] move ChainId to Libra types

2922fe0

christinacelee force-pushed the txn-chain-id branch from 698b214 to 91f2885 Compare July 13, 2020 19:16

christinacelee marked this pull request as ready for review July 13, 2020 19:21

christinacelee requested review from ma2bd and thefallentree as code owners July 13, 2020 19:21

christinacelee changed the title ~~[draft] add ChainID to user txn~~ [types] add ChainID to user txn Jul 13, 2020

[types] add ChainId placeholder field in user txn

4633d27

christinacelee force-pushed the txn-chain-id branch from 91f2885 to 4633d27 Compare July 13, 2020 23:24

christinacelee force-pushed the txn-chain-id branch from ed1b3d9 to 8880386 Compare July 14, 2020 02:28

aching approved these changes Jul 14, 2020

View reviewed changes

sblackshear mentioned this pull request Jul 14, 2020

[move adapter] add verification of chain_id #5076

Closed

[types] change ChainId from u64 to u8

76a7c36

christinacelee force-pushed the txn-chain-id branch from 8880386 to 76a7c36 Compare July 15, 2020 00:11

bors-libra pushed a commit that referenced this pull request Jul 15, 2020

[types] add ChainId placeholder field in user txn

30e3b10

Closes: #4969 Approved by: aching

bors-libra pushed a commit that referenced this pull request Jul 15, 2020

[types] change ChainId from u64 to u8

fbd7352

Closes: #4969 Approved by: aching

bors-libra closed this in b0615b8 Jul 15, 2020

christinacelee deleted the txn-chain-id branch July 15, 2020 01:32

aching added the breaking Any changes that will require restarting the testnet label Jul 15, 2020

christinacelee commented Jul 17, 2020

View reviewed changes

JoshLind mentioned this pull request Jul 18, 2020

[Key Manager] Add ChainID to ConfigBuilder and update docker containers. #5191

Closed

	// Chain ID of the network this transaction is intended for
	// Chain ID of the Libra network this transaction is intended for

	<td>Chain ID of the network this transaction is intended for
	<td>Chain ID of the Libra network this transaction is intended for

	/// chain ID of the network this client is interacting with
	/// chain ID of the Libra network this client is interacting with

[types] add ChainID to user txn #4969

[types] add ChainID to user txn #4969

Conversation

christinacelee commented Jul 8, 2020 • edited

Motivation

Test plan

christinacelee commented Jul 9, 2020

gregnazario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinacelee Jul 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Jul 9, 2020 • edited

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

mimoo Jul 9, 2020 • edited

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

zekun000 left a comment

Choose a reason for hiding this comment

sblackshear commented Jul 9, 2020

zekun000 commented Jul 9, 2020

Choose a reason for hiding this comment

mimoo commented Jul 9, 2020 • edited

sblackshear commented Jul 9, 2020

davidiw commented Jul 9, 2020

sblackshear commented Jul 9, 2020

zekun000 commented Jul 9, 2020

aching commented Jul 9, 2020 • edited

aching commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-libra commented Jul 10, 2020

bors-libra commented Jul 13, 2020

thefallentree commented Jul 13, 2020

thefallentree commented Jul 13, 2020

thefallentree commented Jul 14, 2020

aching 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

aching Jul 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinacelee commented Jul 15, 2020

bors-libra commented Jul 15, 2020

bors-libra commented Jul 15, 2020

github-actions bot commented Jul 15, 2020

bors-libra commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinacelee commented Jul 8, 2020 •

edited

christinacelee Jul 9, 2020 •

edited

mimoo Jul 9, 2020 •

edited

mimoo Jul 9, 2020 •

edited

mimoo commented Jul 9, 2020 •

edited

aching commented Jul 9, 2020 •

edited

aching Jul 14, 2020 •

edited