Network Sustainability Mechanism - ZIP 234 & 235 implementation #8948

mariopil · 2024-10-18T13:14:51Z

Motivation

This PR implements ZIP-234 and ZIP-235.

Specifications & References

Solution

This PR is implemented above #8930 which implements the ZIP-233. It smoothes the issuance curve (ZIP-234) and introduces the split between miner fees (40%) and the amount that is burned (60%, ZIP-235). Some of the code used in the block semantical verification for block subsidy calculation has been moved into the write task, as now the block subsidy calculation relies on the current amount of money_reserve which is obtained from pool value balance at height-1- therefore the previous block must be already present at least in the non-finalized best chain.

Tests

For testing purposes, a local testnet network was set up. It contained two nodes—a zebra and a zcashd one (with the ZIP-234 & 235 implemented, too, PR zcash/zcash#6970). A transaction in a new format containing the burn_amount field was created and added to the block by zcashd. The zebra node synced with zcashd without any errors and accepted the block.

A couple of unit tests were also added.

Follow-up Work

PR Author's Checklist

The PR name will make sense to users.
The PR provides a CHANGELOG summary.
The solution is tested.
The documentation is up to date.
The PR has a priority label.

PR Reviewer's Checklist

The PR Author's checklist is complete.
The PR resolves the issue.

… get_block_template RPC call.

…builder

jackgavigan · 2025-01-16T09:36:40Z

@mariopil Does this PR need to be refreshed when the NSM ZIPs are finalized?

mariopil · 2025-01-16T09:40:28Z

@mariopil Does this PR need to be refreshed when the NSM ZIPs are finalized?

Yes. Currently, the implementation of both 234 and 235 zips is under one feature flag, but they will be covered by separate feature flags.

conradoplg

This is a first pass review, I mostly looked into the feature gating and overall structure

conradoplg · 2025-01-16T13:48:05Z

zebra-chain/src/parameters/network_upgrade.rs

@@ -89,7 +109,10 @@ pub(super) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
    (block::Height(903_000), Heartwood),
    (block::Height(1_046_400), Canopy),
    (block::Height(1_687_104), Nu5),
+    // TODO: Update NU6 activation height once it's been specified.


Suggested change

// TODO: Update NU6 activation height once it's been specified.

conradoplg · 2025-01-16T13:50:01Z

zebra-chain/src/transaction.rs

+    #[cfg(zcash_unstable = "nsm")]
+    pub fn has_burn_amount(&self) -> bool {
+        self.burn_amount() > Amount::<NonNegative>::zero()
+    }


"burn" reference

conradoplg · 2025-01-16T14:22:08Z

zebra-chain/src/transaction/builder.rs

+            // > The NSM burn_amount field [ZIP-233] must be set at minimum to 60% of miner fees [ZIP-235].
+            burn_amount: burn_amount.unwrap_or_else(|| ((miner_fee * 6).unwrap() / 10).unwrap()),


"burn" mentioned

conradoplg · 2025-01-16T14:22:36Z

zebra-chain/src/transaction/serialize.rs

+                let orchard_shielded_data = (&mut limited_reader).zcash_deserialize_into()?;
+
+                // Denoted as `burn_amount` in the spec.
+                let burn_amount = (&mut limited_reader).zcash_deserialize_into()?;


"burn" mentioned

conradoplg · 2025-01-16T14:23:49Z

zebra-chain/src/transaction/unmined.rs

@@ -124,7 +124,7 @@ impl fmt::Display for UnminedTxId {
                .debug_tuple("transaction::Hash")
                .field(&"private")
                .finish(),
-            Witnessed(_id) => f.debug_tuple("WtxId").field(&"private").finish(),
+            Witnessed(id) => f.debug_tuple("WtxId").field(id).finish(),


Suggested change

Witnessed(id) => f.debug_tuple("WtxId").field(id).finish(),

Witnessed(_id) => f.debug_tuple("WtxId").field(&"private").finish(),

Please don't change unrelated code

conradoplg · 2025-01-16T14:30:37Z

zebra-consensus/src/checkpoint.rs

-        // We can't get the block subsidy for blocks with heights in the slow start interval, so we
-        // omit the calculation of the expected deferred amount.
-        let expected_deferred_amount = if height > self.network.slow_start_interval() {
-            // See [ZIP-1015](https://zips.z.cash/zip-1015).
-            funding_stream_values(height, &self.network, block_subsidy(height, &self.network)?)?
-                .remove(&FundingStreamReceiver::Deferred)
-        } else {
-            None
-        };
-


Why was this removed?

Moved to zebra-state/src/service/write.rs write, write_blocks_from_channels().

conradoplg · 2025-01-16T14:31:06Z

zebra-consensus/src/transaction.rs

@@ -444,17 +458,21 @@ where
            // Get the `value_balance` to calculate the transaction fee.
            let value_balance = tx.value_balance(&spent_utxos);

+            let burn_amount = match *tx {


"burn" mentioned

conradoplg · 2025-01-16T14:32:09Z

zebra-consensus/src/transaction.rs

@@ -444,17 +458,21 @@ where
            // Get the `value_balance` to calculate the transaction fee.
            let value_balance = tx.value_balance(&spent_utxos);

+            let burn_amount = match *tx {
+                #[cfg(zcash_unstable = "nsm")]
+                Transaction::ZFuture{ .. } => tx.burn_amount(),


"burn" mentioned - sorry, at this point it's better for you to simply search for all instances of it

Yeah, we've only just submitted PRs to adjust the ZIPs. We've not adjusted this code yet, especially since only 233 is targeted for inclusion in NU7.

conradoplg · 2025-01-16T16:03:03Z

zebra-consensus/src/transaction/tests/prop.rs

@@ -29,7 +29,7 @@ proptest! {
        (network, block_height) in sapling_onwards_strategy(),
        block_time in datetime_full(),
        relative_source_fund_heights in vec(0.0..1.0, 1..=MAX_TRANSPARENT_INPUTS),
-        transaction_version in 4_u8..=5,
+        transaction_version in 4_u8..=6,


Needs to be feature-gated

conradoplg · 2025-01-16T17:19:24Z

zebra-state/src/service/write.rs

@@ -49,15 +55,15 @@ const PARENT_ERROR_MAP_LIMIT: usize = MAX_BLOCK_REORG_HEIGHT as usize * 2;
 pub(crate) fn validate_and_commit_non_finalized(
    finalized_state: &ZebraDb,
    non_finalized_state: &mut NonFinalizedState,
-    prepared: SemanticallyVerifiedBlock,
+    prepared: &mut SemanticallyVerifiedBlock,


Document what is modified in the block

mariopil requested review from a team as code owners October 18, 2024 13:14

mariopil requested review from upbqdn and removed request for a team October 18, 2024 13:14

mpguerra requested review from conradoplg and arya2 October 21, 2024 07:40

arya2 added the do-not-merge Tells Mergify not to merge this PR label Oct 26, 2024

mariopil force-pushed the nsm-zip-234-235 branch from ce2eedd to 4029650 Compare October 30, 2024 15:40

mariopil added 7 commits November 4, 2024 15:53

Network Sustainability Mechanism - ZIP 233 implementation

a5dfc9e

Fixed formatting, added cfg param 'zcash_unstable' for clippy

54b32bd

Added transaction version ZFuture with the burn_amount field into the…

be0ff63

… get_block_template RPC call.

Network Sustainability Mechanism - ZIP 234 and 235 implementation

3e17a38

Added minimum burn_amount calculation to transaction version zfuture …

1776413

…builder

Rpc coinbase outputs calculation fix & tests updates

8500594

Post-rebase fixes.

bb23624

mariopil force-pushed the nsm-zip-234-235 branch from 1ba587c to bb23624 Compare November 5, 2024 11:46

arya2 removed the do-not-merge Tells Mergify not to merge this PR label Dec 7, 2024

mpguerra added the no-review-reminders Turn off review reminders label Jan 13, 2025

conradoplg reviewed Jan 16, 2025

View reviewed changes

oxarbitrage added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Sustainability Mechanism - ZIP 234 & 235 implementation #8948

Network Sustainability Mechanism - ZIP 234 & 235 implementation #8948

mariopil commented Oct 18, 2024

jackgavigan commented Jan 16, 2025 •

edited

Loading

mariopil commented Jan 16, 2025

conradoplg left a comment

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

mariopil Jan 17, 2025

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

giddie Jan 17, 2025 •

edited

Loading

conradoplg Jan 16, 2025

conradoplg Jan 16, 2025

		// > The NSM burn_amount field [ZIP-233] must be set at minimum to 60% of miner fees [ZIP-235].
		burn_amount: burn_amount.unwrap_or_else(\|\| ((miner_fee * 6).unwrap() / 10).unwrap()),

	Witnessed(id) => f.debug_tuple("WtxId").field(id).finish(),
	Witnessed(_id) => f.debug_tuple("WtxId").field(&"private").finish(),

Network Sustainability Mechanism - ZIP 234 & 235 implementation #8948

Are you sure you want to change the base?

Network Sustainability Mechanism - ZIP 234 & 235 implementation #8948

Conversation

mariopil commented Oct 18, 2024

Motivation

Specifications & References

Solution

Tests

Follow-up Work

PR Author's Checklist

PR Reviewer's Checklist

jackgavigan commented Jan 16, 2025 • edited Loading

mariopil commented Jan 16, 2025

conradoplg 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

Choose a reason for hiding this comment

giddie Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackgavigan commented Jan 16, 2025 •

edited

Loading

giddie Jan 17, 2025 •

edited

Loading