Changing add stake operation by ii-cruz · Pull Request #3196 · nimiq/core-rs-albatross

ii-cruz · 2024-12-13T17:07:29Z

What's in this pull request?

Adds the Minimum stake to the add_stake operation. Add stake transactions have an intrinsic check for their value => min_stake and thus are rejected before they can break the invariant.

The idea is that this minimum prevents indefinitely small value add_stake transactions from hindering the retirement.

add_stake assumes that the staker wants to have auto-staking as long as there is an active stake. Once no active stake exists, the funds are credited directly to the inactive balance.

The idea is to prevent DoS of the redelegation of stake.

Adds more unit tests for tombstone and staker interactions, legacy add stake, and new add stake policy
Creates the protocol version gate for the add_stake and intrinsic tx verification
Adapts the web client to pass the protocol version around for tx verification purposes when sending a new tx
Changes the web client by removing the intrinsic transaction checks after receiving the inclusion proofs, since we can rely on the consensus protocol to trust its validity

Pull request checklist

All tests pass. The project builds and runs.
I have resolved any merge conflicts.
I have resolved all clippy and rustfmt warnings.

paberr

Looks good to me

sisou · 2025-03-30T10:57:44Z

When add_stake adds to the inactive balance (when there's no active balance anymore and the inactive one is the biggest), I assume it doesn't reset the inactiveFrom block number?

paberr · 2025-03-30T11:36:15Z

It doesn't reset it.

Also, while discussing with @ii-cruz , one thing that is missing is a version gate for the upgrade.
The new behavior should only be active when we have a new version.

paberr

Generally looks good to me

linear · 2026-05-18T18:07:46Z

T404-159

pkg-pr-new · 2026-05-18T18:12:20Z

Open in StackBlitz

npm i https://pkg.pr.new/@nimiq/core@3196

commit: 813dd79

Copilot

Pull request overview

This PR makes transaction verification protocol-version aware and changes staking add_stake semantics to (a) enforce a minimum stake increment starting at a protocol upgrade version and (b) change how added stake is credited (active-first; otherwise to inactive). It also propagates protocol version through blockchain/mempool/web-client flows and expands the staking test suite (including tombstone/staker interactions and legacy vs upgraded add-stake behavior).

Changes:

Add protocol_version plumbed through Transaction::verify*, BlockState, AbstractBlockchain, mempool verification, and web-client tx verification.
Implement protocol-gated add_stake policy (min increment after upgrade; changed crediting behavior) and add stake receipts/logging to support reverts.
Add/adjust unit tests and docs to cover legacy/new behavior and tombstone/staker interactions.

Reviewed changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
web-client/src/lib.rs	Updates wasm tests to verify transactions under multiple protocol versions.
web-client/src/common/transaction.rs	Adds protocol-version parameter to JS-facing tx verification; changes plain tx validity handling.
web-client/src/client/lib.rs	Fetches protocol version from the client and uses it for tx verification before sending.
wallet/tests/wallet.rs	Updates wallet tx verification calls to pass protocol version.
wallet/tests/multisig.rs	Updates multisig tx verification calls to pass protocol version.
transaction-builder/src/proof/mod.rs	Updates doctests to use protocol-version-aware tx verification.
transaction-builder/src/proof/htlc_contract.rs	Updates doctests to use protocol-version-aware tx verification.
test-utils/src/transactions.rs	Ensures generated staking tx values meet minimum stake for certain staking operations.
test-utils/src/test_custom_block.rs	Updates `BlockState::new` calls to include protocol version.
test-utils/src/performance/accounts-tree/main.rs	Updates `BlockState::new` calls to include protocol version for perf harness.
primitives/transaction/tests/vesting_contract_verify.rs	Updates incoming-tx verification tests for protocol version parameterization.
primitives/transaction/tests/staking_contract_verify.rs	Updates staking incoming-tx verification tests for protocol version parameterization and add-stake min stake policy.
primitives/transaction/tests/htlc_contract_verify.rs	Updates HTLC incoming-tx verification tests for protocol version parameterization.
primitives/transaction/tests/basic_account_verify.rs	Updates basic incoming-tx verification tests for protocol version parameterization.
primitives/transaction/src/lib.rs	Makes `Transaction::verify*` protocol-version aware.
primitives/transaction/src/account/vesting_contract.rs	Adjusts verifier trait implementation signature to accept protocol version.
primitives/transaction/src/account/staking_contract/structs.rs	Enforces protocol-gated minimum add-stake value (and keeps zero-value rejection).
primitives/transaction/src/account/staking_contract/mod.rs	Plumbs protocol version into staking transaction-data verification.
primitives/transaction/src/account/mod.rs	Extends `AccountTransactionVerification` to include protocol version.
primitives/transaction/src/account/htlc_contract.rs	Adjusts verifier trait implementation signature to accept protocol version.
primitives/transaction/src/account/basic_account.rs	Adjusts verifier trait implementation signature to accept protocol version.
primitives/src/policy.rs	Introduces `ADD_STAKE_PROTOCOL_UPGRADE_VERSION`.
primitives/account/tests/vesting_contract.rs	Updates `BlockState::new` calls to include protocol version.
primitives/account/tests/staking_contract/validator.rs	Updates validator tests for new `BlockState::new` and setup helpers carrying protocol version.
primitives/account/tests/staking_contract/staker.rs	Refactors staker tests (removing old add_stake tests) and updates `BlockState` usage; makes helper fns reusable across test modules.
primitives/account/tests/staking_contract/staker_tombstone_interaction.rs	Adds extensive tests for staker/tombstone interactions under new behavior.
primitives/account/tests/staking_contract/staker_add_stake_policy.rs	Adds tests for new add_stake crediting policy and legacy behavior.
primitives/account/tests/staking_contract/mod.rs	Adds new test modules and threads protocol version through setup utilities.
primitives/account/tests/htlc_contract.rs	Updates `BlockState::new` calls to include protocol version.
primitives/account/tests/basic_account.rs	Updates `BlockState::new` calls to include protocol version.
primitives/account/tests/accounts.rs	Updates `BlockState::new` calls and tx verification calls to include protocol version.
primitives/account/src/logs.rs	Extends `Log::Stake` with `credited_balance`.
primitives/account/src/interaction_traits.rs	Extends `BlockState` with `protocol_version` and updates constructor.
primitives/account/src/account/staking_contract/traits.rs	Passes protocol version into `add_stake`; makes add_stake return a receipt for reverts.
primitives/account/src/account/staking_contract/staker.rs	Implements new add_stake crediting policy; introduces add_stake receipts; keeps legacy add_stake logic.
primitives/account/src/account/staking_contract/receipts.rs	Adds `BalanceType` and `AddStakeReceipt`.
mempool/src/verify.rs	Uses chain protocol version when verifying txs in the mempool.
light-blockchain/src/abstract_blockchain.rs	Implements `protocol_version()` for light blockchain.
lib/src/client.rs	Exposes a `protocol_version()` accessor on the client.
blockchain/tests/inherents.rs	Updates `BlockState::new` calls to include protocol version.
blockchain/src/blockchain/wrappers.rs	Uses protocol version in `BlockState` during reserve/release flows.
blockchain/src/blockchain/verify.rs	Verifies txs against the block’s version.
blockchain/src/blockchain/history_sync.rs	Threads `protocol_version` into per-block `BlockState` when applying history items.
blockchain/src/blockchain/accounts.rs	Threads `protocol_version` into `BlockState` for commit/revert.
blockchain/src/blockchain/abstract_blockchain.rs	Implements `protocol_version()` for full blockchain.
blockchain/src/block_production/mod.rs	Threads protocol version into produced blocks’ execution `BlockState`.
blockchain-proxy/src/blockchain_proxy.rs	Forwards `protocol_version()` through blockchain proxy.
blockchain-interface/src/abstract_blockchain.rs	Adds `protocol_version()` to the `AbstractBlockchain` trait.

Comments suppressed due to low confidence (1)

primitives/account/tests/staking_contract/staker_add_stake_policy.rs:446

This function is annotated with #[test] but takes an argument (protocol_version: u16), which will not compile (Rust test functions must be fn()). Make this a helper function without #[test], or add a parameterless #[test] wrapper that calls it with the intended protocol version(s).

/// Adding stake in absence of active balance should credit the inactive balance irrespective of the retired balance.
#[test]
fn add_stake_with_only_retired_balance(protocol_version: u16) {
    // -----------------------------------

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    pub fn verify_mut(
+        &mut self,
+        network_id: NetworkId,
+        protocol_version: u16,
+    ) -> Result<(), TransactionError> {
+        let ret = self.verify(network_id, protocol_version);
        if ret.is_ok() {
            self.valid = true;
        }
        ret
    }

-    pub fn verify(&self, network_id: NetworkId) -> Result<(), TransactionError> {
+    pub fn verify(
+        &self,
+        network_id: NetworkId,
+        protocol_version: u16,
+    ) -> Result<(), TransactionError> {
        if self.valid {
            return Ok(());
        }



                self.revert_create_staker(&mut store, &staker_address, transaction.value, tx_logger)
            }
            IncomingStakingTransactionData::AddStake { staker_address } => {
-                self.revert_add_stake(&mut store, &staker_address, transaction.value, tx_logger)
+                let receipt = receipt.ok_or(AccountError::InvalidReceipt)?.try_into()?;
+
+                self.revert_add_stake(
+                    &mut store,
+                    &staker_address,
+                    transaction.value,
+                    receipt,
+                    tx_logger,
+                )


+                    block_state.protocol_version,
+                    tx_logger,
+                )
+                .map(|receipt| Some(receipt.into())),


+        // Add stake txs never violate minimum stake for the non-retired funds (invariant 1),
+        // because the intrinsic tx checks that value is >= min stake.
+        assert!(
+            Staker::enforce_min_stake(
+                staker.active_balance + value,
+                staker.inactive_balance,
+                staker.retired_balance,
+            )
+            .is_ok(),
+            "Add stake should never violate the min stake invariants"
+        );


                    };
                    proof.to_plain_transaction_proof()
                }
            },
            size: self.serialized_size(),
-            valid: self.verify(None).is_ok(),
+            valid: false,
        }


+    // 1. Acquire blockchain read lock
    let blockchain = blockchain.read();

+    // 2. Verify transaction signature (and other stuff)
+    transaction.verify_mut(network_id, blockchain.state.current_version())?;
+
    // 3. Check validity window and already included


…o the bigger between inactive and retired.

Add tests for legacy add_stake.

…bstone stake interactions

Refactoring tests

…tructor

ii-cruz added the bug Something isn't working label Dec 13, 2024

ii-cruz self-assigned this Dec 13, 2024

ii-cruz force-pushed the iicruz/fix-staker-dos branch from e62eccb to 94256d9 Compare January 7, 2025 14:06

paberr approved these changes Mar 30, 2025

View reviewed changes

Comment thread primitives/account/src/account/staking_contract/staker.rs Outdated

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 94256d9 to bfb490c Compare August 1, 2025 18:16

ii-cruz force-pushed the iicruz/fix-staker-dos branch 9 times, most recently from 9d0d70d to 91342a2 Compare August 22, 2025 14:23

ii-cruz marked this pull request as ready for review August 22, 2025 16:15

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 91342a2 to 2573ee9 Compare August 22, 2025 16:16

ii-cruz added this to the Next upgrade milestone Aug 22, 2025

ii-cruz added the Protocol Upgrade label Aug 22, 2025

ii-cruz requested a review from paberr August 22, 2025 16:19

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 2573ee9 to 8ec152e Compare August 25, 2025 14:19

paberr reviewed Aug 27, 2025

View reviewed changes

Comment thread primitives/account/tests/staking_contract/add_stake_policy.rs

Comment thread primitives/transaction/src/account/staking_contract/structs.rs

ii-cruz force-pushed the iicruz/fix-staker-dos branch 5 times, most recently from 1b0174e to f480b1e Compare September 1, 2025 15:47

ii-cruz requested review from paberr and sisou September 1, 2025 15:55

ii-cruz force-pushed the iicruz/fix-staker-dos branch 2 times, most recently from 1ee5e09 to 6b12336 Compare September 10, 2025 12:34

ii-cruz force-pushed the iicruz/fix-staker-dos branch from d8417e1 to e38e579 Compare September 18, 2025 11:47

ii-cruz force-pushed the iicruz/fix-staker-dos branch from c1fa888 to 9a6bf04 Compare October 6, 2025 16:56

Copilot AI review requested due to automatic review settings May 18, 2026 18:07

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 3c4439a to 5c49cd2 Compare May 18, 2026 18:07

Copilot started reviewing on behalf of ii-cruz May 18, 2026 18:08 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 5c49cd2 to 6410aa5 Compare May 28, 2026 16:50

ii-cruz added 15 commits May 28, 2026 17:52

Fixing web client flaky tests

fb25472

Add stake adds to active if it's predominant on the active stake or t…

c946cd0

…o the bigger between inactive and retired.

Adding minimum stake restriction to the add stake.

1b55810

Change crediting policy when adding stake. Adds respective unit tests.

013d803

Integrating version the upgrade.

d6a2551

Passing the version around

3157907

Call legacy add stake in depending on version.

1c02996

Fixing the tests

04af6ad

Adding upgrade version

77e2ed1

Add protocol support to staker and validator test helps.

9b89e31

Add tests for legacy add_stake.

Adding protocol_version to intrinsic tx verification

2abeed6

Fixing the add stake for tombstone behaviour and adding tests for tom…

3d2d179

…bstone stake interactions

Changing add stake policy to credit only active or inactive balances

5496c4f

Renaming add_stake legacy to include version.

2648552

Refactoring tests

Removing the protocol version argument from the PlainTransaction cons…

de17169

…tructor

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 6410aa5 to de17169 Compare May 28, 2026 16:53

ii-cruz force-pushed the iicruz/fix-staker-dos branch from 9192f0d to a0f27b8 Compare June 8, 2026 14:45

Fix the web client tests

813dd79

ii-cruz force-pushed the iicruz/fix-staker-dos branch from a0f27b8 to 813dd79 Compare June 8, 2026 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing add stake operation#3196

Changing add stake operation#3196
ii-cruz wants to merge 16 commits into
albatrossfrom
iicruz/fix-staker-dos

ii-cruz commented Dec 13, 2024 •

edited

Loading

Uh oh!

paberr left a comment

Uh oh!

Uh oh!

sisou commented Mar 30, 2025

Uh oh!

paberr commented Mar 30, 2025

Uh oh!

paberr left a comment

Uh oh!

Uh oh!

Uh oh!

linear Bot commented May 18, 2026

Uh oh!

pkg-pr-new Bot commented May 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ii-cruz commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Pull request checklist

Uh oh!

paberr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sisou commented Mar 30, 2025

Uh oh!

paberr commented Mar 30, 2025

Uh oh!

paberr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

linear Bot commented May 18, 2026

Uh oh!

pkg-pr-new Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ii-cruz commented Dec 13, 2024 •

edited

Loading

pkg-pr-new Bot commented May 18, 2026 •

edited

Loading