Introduce CommitOnly variant of Inputs #3419

antiochp · 2020-08-08T14:17:50Z

This PR bumps the node protocol version to to 3.

Inputs in both transactions and blocks are serialized as follows -

(Commitment)

Previously in protocol version 2 they were serialized as -

(Features || Commitment)

Note that the sorting of inputs is affected by this also.

Inputs are now sorted using the following sort key -

Hash(Commitment)

Previously they were sorted with the key -

Hash(Features || Commitment)

This affects the serialization of lists of inputs, not just individual inputs themselves. Inputs are not stored on-chain so this is not a consensus breaking change.
Outputs are unaffected by this change.

Full support for protocol version 2 peers is maintained via the existing version negotiation scheme.

Locally we store full blocks in the db in v3 format. These are converted to v2 before broadcasting to v2 peers as necessary.

Transactions are stored in the txpool (and stempool) in v2 format. This is to allow broadcast and relay with v2 peers in a convenient way. It is non-trivial to convert transactions v3->v2 due to possibility of one transaction spending unconfirmed outputs from another transaction in the txpool. To simplify transaction handling we maintain them in v2 format.
This will be revisited once we have wide support for v3 across the network.

Care has been taken not to introduce malleability here with backward support. Blocks and transactions in v2 format must have a valid and correct features byte.

Why Make This Change?

Uniqueness is enforced across the current utxo set.
It is not valid to have two unspent outputs with the same commitment.
Spending an output is done solely by output commitment. The features bytes is entirely redundant.
Internally we want to simplify the spending logic by removing any reference to the redundant input features byte.

This is one of the outstanding wrinkles in the p2p msg format that we wish to resolve prior to the final scheduled hardfork.
This leaves open the possibility of taking advantage of the final HF to break backward compatibility, simplifying a lot of internal version handling implementation details.

Introduce CommitOnly variant of Inputs
Introduce CommitWrapper to wrap "commit only" inputs so we can sort them correctly (by hash).
v3 -> v2 conversion (need to lookup outputs being spent) for backward compatibility with v2 peers

TODO -

grin-wallet needs some rework to support this (handle transaction inputs correctly grin-wallet#512)

Introduce CommitWrapper so we can sort commit only inputs correctly.

convert to v2 when relaying to v2 peers

quentinlesceller

Wow took me a while to review. Looking good. Few comments below. Also ran it locally for 24 hours without issues.

quentinlesceller · 2020-09-04T15:34:03Z

chain/src/chain.rs

@@ -171,6 +171,10 @@ impl Chain {
 	) -> Result<Chain, Error> {
 		let store = Arc::new(store::ChainStore::new(&db_root)?);

+		// DB migrations to be run prior to the chain being used.
+		// Migrate full blocks to protocol version v3.
+		Chain::migrate_db_v2_v3(&store)?;


Any reason why don't need a lexical scope anymore?

It was purely for visual/readability purposes before. It was not needed at the code level.

quentinlesceller · 2020-09-04T15:46:52Z

chain/src/chain.rs

@@ -291,18 +290,19 @@ impl Chain {

 		let mut header_pmmr = self.header_pmmr.write();
 		let mut txhashset = self.txhashset.write();
-		let outputs =
+		let inputs: Vec<_> =


I would find it easier to ready if inputs was named unspent_outputs.

chain/src/txhashset/utxo_view.rs

quentinlesceller · 2020-09-04T16:04:22Z

chain/src/chain.rs

@@ -662,12 +663,12 @@ impl Chain {

 	/// Verify we are not attempting to spend a coinbase output
 	/// that has not yet sufficiently matured.
-	pub fn verify_coinbase_maturity(&self, tx: &Transaction) -> Result<(), Error> {
+	pub fn verify_coinbase_maturity(&self, inputs: Inputs) -> Result<(), Error> {


Wondering if is it any better to change the function signature to:

pub fn verify_coinbase_maturity(&self, inputs: &Inputs) -> Result<(), Error> {

In the current case Rust makes a copy of the Input no?

👍 I think this makes sense. Same for verify_inputs().

quentinlesceller · 2020-09-04T16:12:24Z

chain/src/txhashset/utxo_view.rs

 								if out == input.into() {
 									Ok((out, pos))
 								} else {
+									error!("input mismatch: {:?}, {:?}, {:?}", out, pos, input);
 									Err(ErrorKind::Other("input mismatch".into()).into())
 								}


So is this check redundant based on what we have at line 128-136 or the OutputFeatures feature check is important (for Inputs::FeaturesAndCommit variant only)?

Yes, the latter. We need to ensure that if the features are passed in that they are correct.
For CommitOnly variant we can safely skip this.

This is to maintain existing behavior and to avoid introducing malleability with the features being effectively ignored.

pool/src/transaction_pool.rs

pool/src/pool.rs

core/src/core/transaction.rs

antiochp mentioned this pull request Aug 8, 2020

[WIP] v3 blocks and transactions (no redundant input features) #3385

Closed

18 tasks

antiochp force-pushed the inputs_enum_variants branch 2 times, most recently from 87f2b89 to 8c06ece Compare August 10, 2020 09:08

antiochp changed the title ~~Introduce CommitOnly variant of Inputs.~~ Introduce CommitOnly variant of Inputs Aug 10, 2020

antiochp marked this pull request as ready for review August 21, 2020 07:01

antiochp force-pushed the inputs_enum_variants branch from e40b54b to 0167a8d Compare August 21, 2020 07:07

antiochp added the enhancement label Aug 21, 2020

This was referenced Aug 24, 2020

handle transaction inputs correctly mimblewimble/grin-wallet#512

Merged

P2P Protocol Version for HF4 #3430

Open

antiochp added 15 commits September 3, 2020 13:39

Introduce CommitOnly variant of Inputs.

496a698

Introduce CommitWrapper so we can sort commit only inputs correctly.

rememebr to resort if converting

d6854f8

write inputs based on variant and protocol version

f4dcfb2

read and write protocol version specific inputs

0d152f3

store full blocks in local db in v3

3a68648

convert to v2 when relaying to v2 peers

add debug version_str for inputs

0414191

no assumptions about spent index sort order

4abd8fd

add additional version debug logs

dae8a6d

fix ser/deser tests for proto v3

cbfec31

cleanup coinbase maturity

758cc1b

rework pool to better handle v2 conversion robustly

a3c3af9

cleanup txpool add_to_pool

610aa0b

fix nrd kernel test

7835989

move init conversion earlier

58d9760

cleanup

a5be223

antiochp force-pushed the inputs_enum_variants branch from d81c22c to a5be223 Compare September 3, 2020 13:29

antiochp requested a review from quentinlesceller September 3, 2020 17:07

quentinlesceller approved these changes Sep 4, 2020

View reviewed changes

cleanup based on PR feedback

b5dae01

antiochp force-pushed the inputs_enum_variants branch from 7c32e4e to b5dae01 Compare September 7, 2020 09:06

antiochp merged commit 7dc9457 into mimblewimble:master Sep 7, 2020

antiochp deleted the inputs_enum_variants branch September 7, 2020 15:58

antiochp added this to the 4.1.0 milestone Sep 9, 2020

antiochp mentioned this pull request Sep 16, 2020

4.1.0 Release Notes #3445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CommitOnly variant of Inputs #3419

Introduce CommitOnly variant of Inputs #3419

antiochp commented Aug 8, 2020 •

edited

Loading

quentinlesceller left a comment •

edited

Loading

quentinlesceller Sep 4, 2020

antiochp Sep 7, 2020

quentinlesceller Sep 4, 2020

quentinlesceller Sep 4, 2020

antiochp Sep 7, 2020

quentinlesceller Sep 4, 2020

antiochp Sep 7, 2020 •

edited

Loading

Introduce CommitOnly variant of Inputs #3419

Introduce CommitOnly variant of Inputs #3419

Conversation

antiochp commented Aug 8, 2020 • edited Loading

Why Make This Change?

quentinlesceller left a comment • edited Loading

Choose a reason for hiding this comment

quentinlesceller Sep 4, 2020

Choose a reason for hiding this comment

antiochp Sep 7, 2020

Choose a reason for hiding this comment

quentinlesceller Sep 4, 2020

Choose a reason for hiding this comment

quentinlesceller Sep 4, 2020

Choose a reason for hiding this comment

antiochp Sep 7, 2020

Choose a reason for hiding this comment

quentinlesceller Sep 4, 2020

Choose a reason for hiding this comment

antiochp Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

antiochp commented Aug 8, 2020 •

edited

Loading

quentinlesceller left a comment •

edited

Loading

antiochp Sep 7, 2020 •

edited

Loading