From 1ac9b21189f91f487267cf3d4941d69d56570452 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 23 Dec 2021 19:48:49 -0300 Subject: [PATCH 1/4] improve docs of chain value balances non-negative consensus rules --- zebra-chain/src/value_balance.rs | 49 +++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/zebra-chain/src/value_balance.rs b/zebra-chain/src/value_balance.rs index 7e05808b326..ca21a71be63 100644 --- a/zebra-chain/src/value_balance.rs +++ b/zebra-chain/src/value_balance.rs @@ -142,13 +142,15 @@ where } impl ValueBalance { - /// [Consensus rule]: The remaining value in the transparent transaction value pool MUST - /// be nonnegative. + /// # Consensus + /// + /// > The remaining value in the transparent transaction value pool MUST be nonnegative. + /// + /// /// /// This rule applies to Block and Mempool transactions. /// - /// [Consensus rule]: https://zips.z.cash/protocol/protocol.pdf#transactions - /// Design: https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0012-value-pools.md#definitions + /// Design: // // TODO: move this method to Transaction, so it can handle coinbase transactions as well? pub fn remaining_transaction_value(&self) -> Result, amount::Error> { @@ -165,17 +167,36 @@ impl ValueBalance { /// `utxos` must contain the [`Utxo`]s of every input in this block, /// including UTXOs created by earlier transactions in this block. /// - /// "If any of the "Sprout chain value pool balance", "Sapling chain value pool balance", - /// or "Orchard chain value pool balance" would become negative in the block chain created - /// as a result of accepting a block, then all nodes MUST reject the block as invalid. + /// # Consensus + /// + /// > If the Sprout chain value pool balance would become negative in the block chain + /// > created as a result of accepting a block, then all nodes MUST reject the block as invalid. + /// + /// + /// + /// > If the Sapling chain value pool balance would become negative in the block chain + /// > created as a result of accepting a block, then all nodes MUST reject the block as invalid. /// - /// Nodes MAY relay transactions even if one or more of them cannot be mined due to the - /// aforementioned restriction." + /// /// - /// https://zips.z.cash/zip-0209#specification + /// > If the Orchard chain value pool balance would become negative in the block chain + /// > created as a result of accepting a block , then all nodes MUST reject the block as invalid. + /// + /// /// /// Zebra also checks that the transparent value pool is non-negative, /// which is a consensus rule derived from Bitcoin. + /// Individual transactions have a value pool that must be non-negative, + /// so the Transparent chain value pool balance (which is the sum of all the value pools for each + /// transaction and for each block in the chain) must be non-negative too. + /// > The remaining value in the transparent transaction value pool MUST be nonnegative. + /// + /// + /// + /// > Nodes MAY relay transactions even if one or more of them cannot be mined due to the + /// > aforementioned restriction." + /// + /// /// /// Note: the chain value pool has the opposite sign to the transaction /// value pool. @@ -261,13 +282,7 @@ impl ValueBalance { .expect("conversion from NonNegative to NegativeAllowed is always valid"); chain_value_pool = (chain_value_pool + chain_value_pool_change)?; - // Consensus rule: If any of the "Sprout chain value pool balance", - // "Sapling chain value pool balance", or "Orchard chain value pool balance" - // would become negative in the block chain created as a result of accepting a block, - // then all nodes MUST reject the block as invalid. - // - // Zebra also checks that the transparent value pool is non-negative, - // which is a consensus rule derived from Bitcoin. + // This will error if the chain value pool balance gets negative with the change. chain_value_pool.constrain() } From e7de441b33c096f41ab6090a7df0f2b59a05ab18 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 28 Dec 2021 16:29:53 -0300 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: teor --- zebra-chain/src/value_balance.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/zebra-chain/src/value_balance.rs b/zebra-chain/src/value_balance.rs index ca21a71be63..1e051ac2412 100644 --- a/zebra-chain/src/value_balance.rs +++ b/zebra-chain/src/value_balance.rs @@ -151,8 +151,6 @@ impl ValueBalance { /// This rule applies to Block and Mempool transactions. /// /// Design: - // - // TODO: move this method to Transaction, so it can handle coinbase transactions as well? pub fn remaining_transaction_value(&self) -> Result, amount::Error> { // Calculated by summing the transparent, sprout, sapling, and orchard value balances, // as specified in: @@ -189,12 +187,13 @@ impl ValueBalance { /// Individual transactions have a value pool that must be non-negative, /// so the Transparent chain value pool balance (which is the sum of all the value pools for each /// transaction and for each block in the chain) must be non-negative too. + /// /// > The remaining value in the transparent transaction value pool MUST be nonnegative. /// /// /// /// > Nodes MAY relay transactions even if one or more of them cannot be mined due to the - /// > aforementioned restriction." + /// > aforementioned restriction. /// /// /// From b7c29af239e9854410902af7de0ed8cd101ee2ba Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 28 Dec 2021 17:08:51 -0300 Subject: [PATCH 3/4] move comment --- zebra-chain/src/value_balance.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/value_balance.rs b/zebra-chain/src/value_balance.rs index 1e051ac2412..a8fc9419b9a 100644 --- a/zebra-chain/src/value_balance.rs +++ b/zebra-chain/src/value_balance.rs @@ -155,6 +155,7 @@ impl ValueBalance { // Calculated by summing the transparent, sprout, sapling, and orchard value balances, // as specified in: // https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#definitions + // This will error if the chain value pool balance gets negative with the change. (self.transparent + self.sprout + self.sapling + self.orchard)?.constrain::() } } @@ -208,6 +209,7 @@ impl ValueBalance { ) -> Result, ValueBalanceError> { let chain_value_pool_change = block.borrow().chain_value_pool_change(utxos)?; + // This will error if the chain value pool balance gets negative with the change. self.add_chain_value_pool_change(chain_value_pool_change) } @@ -281,7 +283,6 @@ impl ValueBalance { .expect("conversion from NonNegative to NegativeAllowed is always valid"); chain_value_pool = (chain_value_pool + chain_value_pool_change)?; - // This will error if the chain value pool balance gets negative with the change. chain_value_pool.constrain() } From a4e401abe1abc1a89ff1f2cced91f02292125076 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 30 Dec 2021 10:27:53 +1000 Subject: [PATCH 4/4] Fix confusion between chain and transaction value pools --- zebra-chain/src/value_balance.rs | 58 +++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/zebra-chain/src/value_balance.rs b/zebra-chain/src/value_balance.rs index a8fc9419b9a..a2716422704 100644 --- a/zebra-chain/src/value_balance.rs +++ b/zebra-chain/src/value_balance.rs @@ -142,6 +142,9 @@ where } impl ValueBalance { + /// Assumes that this value balance is a transaction value balance, + /// and returns the remaining value in the transaction value pool. + /// /// # Consensus /// /// > The remaining value in the transparent transaction value pool MUST be nonnegative. @@ -155,17 +158,23 @@ impl ValueBalance { // Calculated by summing the transparent, sprout, sapling, and orchard value balances, // as specified in: // https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#definitions - // This will error if the chain value pool balance gets negative with the change. + // + // This will error if the remaining value in the transaction value pool is negative. (self.transparent + self.sprout + self.sapling + self.orchard)?.constrain::() } } impl ValueBalance { - /// Return the sum of the chain value pool change from `block`, and this value balance. + /// Returns the sum of this value balance, and the chain value pool changes in `block`. /// /// `utxos` must contain the [`Utxo`]s of every input in this block, /// including UTXOs created by earlier transactions in this block. /// + /// Note: the chain value pool has the opposite sign to the transaction + /// value pool. + /// + /// See [`Block::chain_value_pool_change`] for details. + /// /// # Consensus /// /// > If the Sprout chain value pool balance would become negative in the block chain @@ -183,25 +192,22 @@ impl ValueBalance { /// /// /// - /// Zebra also checks that the transparent value pool is non-negative, - /// which is a consensus rule derived from Bitcoin. - /// Individual transactions have a value pool that must be non-negative, - /// so the Transparent chain value pool balance (which is the sum of all the value pools for each - /// transaction and for each block in the chain) must be non-negative too. + /// > If any of the "Sprout chain value pool balance", "Sapling chain value pool balance", or + /// > "Orchard chain value pool balance" would become negative in the block chain created + /// > as a result of accepting a block, then all nodes MUST reject the block as invalid. /// - /// > The remaining value in the transparent transaction value pool MUST be nonnegative. + /// /// - /// + /// Zebra also checks that the transparent value pool is non-negative. + /// In Zebra, we define this pool as the sum of all unspent transaction outputs. + /// (Despite their encoding as an `int64`, transparent output values must be non-negative.) /// - /// > Nodes MAY relay transactions even if one or more of them cannot be mined due to the - /// > aforementioned restriction. + /// This is a consensus rule derived from Bitcoin: /// - /// + /// > because a UTXO can only be spent once, + /// > the full value of the included UTXOs must be spent or given to a miner as a transaction fee. /// - /// Note: the chain value pool has the opposite sign to the transaction - /// value pool. - /// - /// See [`Block::chain_value_pool_change`] for details. + /// pub fn add_block( self, block: impl Borrow, @@ -213,7 +219,7 @@ impl ValueBalance { self.add_chain_value_pool_change(chain_value_pool_change) } - /// Return the sum of the chain value pool change from `transaction`, and this value balance. + /// Returns the sum of this value balance, and the chain value pool changes in `transaction`. /// /// `outputs` must contain the [`Output`]s of every input in this transaction, /// including UTXOs created by earlier transactions in its block. @@ -223,6 +229,20 @@ impl ValueBalance { /// /// See [`Block::chain_value_pool_change`] and [`Transaction::value_balance`] /// for details. + /// + /// # Consensus + /// + /// > If any of the "Sprout chain value pool balance", "Sapling chain value pool balance", or + /// > "Orchard chain value pool balance" would become negative in the block chain created + /// > as a result of accepting a block, then all nodes MUST reject the block as invalid. + /// > + /// > Nodes MAY relay transactions even if one or more of them cannot be mined due to the + /// > aforementioned restriction. + /// + /// + /// + /// Since this consensus rule is optional for mempool transactions, + /// Zebra does not check it in the mempool transaction verifier. #[cfg(any(test, feature = "proptest-impl"))] pub fn add_transaction( self, @@ -241,7 +261,7 @@ impl ValueBalance { self.add_chain_value_pool_change(chain_value_pool_change) } - /// Return the sum of the chain value pool change from `input`, and this value balance. + /// Returns the sum of this value balance, and the chain value pool change in `input`. /// /// `outputs` must contain the [`Output`] spent by `input`, /// (including UTXOs created by earlier transactions in its block). @@ -268,7 +288,7 @@ impl ValueBalance { self.add_chain_value_pool_change(transparent_value_pool_change) } - /// Return the sum of the chain value pool change, and this value balance. + /// Returns the sum of this value balance, and the `chain_value_pool_change`. /// /// Note: the chain value pool has the opposite sign to the transaction /// value pool.