Skip to content

fix(consensus): accept justified peer block on #1e by default (no env) #801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
github-actions merged 2 commits into from
Jun 6, 2026
Merged

fix(consensus): accept justified peer block on #1e by default (no env) #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a420286
fix(consensus): accept justified peer block on #1e by default (no env)
satyakwok Jun 6, 2026
c204259
fix(consensus): plumb Pass-1 justification-verified flag into #1e acc…
satyakwok Jun 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ members = [".", "crates/sentrix-primitives", "crates/sentrix-wallet", "crates/se
# `version.workspace = true`. Same goes for edition/license/repository so
# they can't drift across crates.
[workspace.package]
version = "2.2.40"
version = "2.2.41"
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider version-bump semantics for consensus behavior changes.

The workspace version was incremented by a patch level (2.2.40 → 2.2.41). While the PR states "no fork gate is required" because only the accept/reject decision changes for already-canonical blocks, this PR does alter the default consensus behavior by removing environment-variable gating and making justified peer block acceptance uniform across all nodes.

For blockchain systems, behavioral changes in consensus logic—even when deterministic and backward-compatible at the state level—may warrant a minor version bump to signal the operational change to node operators, especially when it affects validator/fullnode behavior divergence.

Verify that the project's versioning policy allows patch bumps for consensus behavior changes that don't require fork gates.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Cargo.toml` at line 10, The PR changes default consensus behavior but only
updated the workspace version from "2.2.40" to "2.2.41"; check the project's
versioning policy to confirm whether consensus-behavior changes require a minor
bump, and if so update the version field (currently set via the version =
"2.2.41" entry) to the next minor (e.g., 2.3.0), update the changelog/release
notes to call out the consensus behavior change, and adjust the PR
title/description and commit message to reflect a minor-semver bump so operators
are properly signaled; if policy allows a patch, add an explicit note in the
changelog explaining why a patch bump was used.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how SENTRIX_OBSERVER_TOLERANT_STATE_ROOT is handled in block_executor

# Search for references to the env var in block_executor
echo "=== Checking SENTRIX_OBSERVER_TOLERANT_STATE_ROOT in block_executor ==="
rg -nC5 'SENTRIX_OBSERVER_TOLERANT_STATE_ROOT' crates/sentrix-core/src/block_executor.rs

# Check if any env var reads still exist
echo -e "\n=== Checking for env::var reads in block_executor ==="
rg -nC3 'env::var.*SENTRIX_OBSERVER' crates/sentrix-core/src/

# Verify the env var is mentioned in comments or kept as no-op
echo -e "\n=== Checking if variable is documented as ignored ==="
rg -nC2 'OBSERVER.*TOLERANT.*ignore|ignore.*OBSERVER.*TOLERANT' crates/sentrix-core/src/

Repository: sentrix-labs/sentrix

Length of output: 246

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Find all references to the env var anywhere in the repo
echo "=== All references to SENTRIX_OBSERVER_TOLERANT_STATE_ROOT ==="
rg -n --hidden --glob '!.git/*' 'SENTRIX_OBSERVER_TOLERANT_STATE_ROOT' . || true

# 2) Find all env reads that mention OBSERVER or TOLERANT near them
echo -e "\n=== env::var reads mentioning OBSERVER/TOLERANT ==="
rg -n --hidden --glob '!.git/*' 'env::var\([^)]*(OBSERVER|TOLERANT)[^)]*\)|std::env::var\([^)]*(OBSERVER|TOLERANT)[^)]*\)' crates/ . || true

# 3) Look for docs/config mentioning the env var
echo -e "\n=== Mentions in README/docs/config ==="
rg -n --hidden --glob '!.git/*' 'SENTRIX_OBSERVER_TOLERANT_STATE_ROOT' -S README* docs* .github* config* crates/ || true

Repository: sentrix-labs/sentrix

Length of output: 332

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 4) Specifically inspect fullnode config parsing code for references/removal patterns
# (adjusted by searching for "fullnode" + env::var usage)
echo "=== Search for fullnode config env handling patterns ==="
rg -n --hidden --glob '!.git/*' 'fullnode' crates/ | head -n 50 || true

echo -e "\n=== Search for env var handling in fullnode-related code ==="
rg -n --hidden --glob '!.git/*' '(env::var|std::env::var)' crates/ | rg -n '(fullnode|config|observer|tolerant)' || true

Repository: sentrix-labs/sentrix

Length of output: 1715

Preserve SENTRIX_OBSERVER_TOLERANT_STATE_ROOT as an ignored/no-op env var (it’s currently gone)

SENTRIX_OBSERVER_TOLERANT_STATE_ROOT has no references anywhere in the repo (including crates/sentrix-core/src/block_executor.rs), so it appears to be deleted rather than kept as an ignored/no-op env var. This conflicts with the consensus-critical guideline to not delete existing env vars/features—either retain the env var read (even if it no longer affects behavior) or document a clear deprecation/removal rationale and ensure the fullnode config follow-up is consistent.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Cargo.toml` at line 10, The environment variable
SENTRIX_OBSERVER_TOLERANT_STATE_ROOT was removed instead of being preserved as a
no-op; restore a benign read of SENTRIX_OBSERVER_TOLERANT_STATE_ROOT (e.g.,
getenv and ignore or set to an unused local) in the initialization path that
previously referenced it (look for block executor or observer initialization
code such as functions in block_executor.rs or any observer bootstrap function)
so the env var remains accepted but has no behavioral effect, or alternatively
add a clear deprecation comment and a documented removal note in the code and
configuration docs and ensure the fullnode config handling reflects the removal;
update the code to read the var and ignore its value or add the explicit
deprecation handling where SENTRIX_OBSERVER_TOLERANT_STATE_ROOT was formerly
used.

Source: Coding guidelines

edition = "2024"
license = "BUSL-1.1"
repository = "https://github.com/sentrix-labs/sentrix"
Expand Down
159 changes: 131 additions & 28 deletions crates/sentrix-core/src/block_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ impl Blockchain {
}

fn add_block_impl(&mut self, block: Block) -> SentrixResult<()> {
// Reset per-add: only the justification gate below sets this true.
self.current_add_justification_verified = false;
let expected_index = self.height() + 1;
let expected_prev = self.latest_block()?.hash.clone();

Expand Down Expand Up @@ -368,6 +370,11 @@ impl Blockchain {
j.signer_count(),
)));
}
// Gate ran AND passed (no early-return above) → record that this
// block's 2/3 justification was actually verified, for Pass-2's
// #1e accept. A justification blob alone is insufficient — it could
// have bypassed this gate via replay / pre-voyager (CodeRabbit #801).
self.current_add_justification_verified = true;
}

// C-04: validate coinbase amount AND recipient. Amount must equal the
Expand Down Expand Up @@ -1727,30 +1734,44 @@ impl Blockchain {
return Ok(());
}

// Observer-tolerant accept (gated, default OFF). An observer/
// fullnode applies EVERY block via add_block_from_peer (Peer) and
// strictly rejecting a #1e here halts it on canonical data: the
// block already passed the strict 2/3-precommit justification
// verification earlier in add_block_impl, so it IS the network-
// agreed block (consensus is on block_hash, not state_root). The
// mismatch is the chain's known imperfect state-commitment
// (recurring/oscillating state_root) that validators already
// tolerate via the apply-from-stash path. With
// SENTRIX_OBSERVER_TOLERANT_STATE_ROOT=1 set, accept the block and
// stamp the proposer's (canonical) received root so the observer's
// chain stays consistent with the committed roots; its local
// accounts diverge from that root (the same pre-existing imperfection
// every node has), so served state is no worse than a validator's.
// Default OFF → validators keep the strict #1e reject below. Only an
// observer node sets this env.
// #1e on a Peer block whose 2/3 justification was
// VERIFIED in Pass-1 → ACCEPT (stamp the proposer's
// canonical root). `current_add_justification_verified`
// is set only when the gate in add_block_impl (~232)
// actually ran AND passed — NOT merely when a
// justification blob is present, which could have
// bypassed the gate via replay / pre-voyager
// (CodeRabbit #801). Consensus is on block_hash, not
// state_root, so a gate-verified block IS network-
// canonical; the local-recompute mismatch is
// the chain's known imperfect state-commitment
// (recurring/oscillating state_root) that the
// apply-from-stash path already tolerates. Strictly
// rejecting it halts the node on canonical data — the
// lagging-validator stuck-sync (val1, 2026-06-06,
// recovered only by halt-all). This is the DEFAULT
// for ALL nodes now, replacing the per-node
// SENTRIX_OBSERVER_TOLERANT env whose validator-vs-
// fullnode divergence caused that stall. The node's
// local accounts diverge from the stamped root (the
// same imperfection every node carries), so served
// state is no worse than any validator's. A Peer
// block with NO justification is NOT accepted here —
// it falls through to the strict reject below.
//
// (Cryptographic per-signature justification
// verification — STRICT_JUSTIFICATION_HEIGHT — is a
// separate, currently-broken path: recover_signer
// disagrees with the producer's signing payload, so
// it stays off; this accept inherits the chain's
// existing justification trust level, no weaker.)
if self.source_for_current_add == BlockSource::Peer
&& std::env::var_os("SENTRIX_OBSERVER_TOLERANT_STATE_ROOT")
.is_some_and(|v| v == "1")
&& self.current_add_justification_verified
{
tracing::debug!(
"observer-tolerant: #1e at block {} (received {} vs computed \
{}) — accepting justified canonical block, stamping received \
root (local state diverges; chain state-commitment imperfect)",
"#1e accept at block {} (received {} vs computed {}) — \
justified canonical block, stamping received root \
(local state diverges; chain state-commitment imperfect)",
block_index,
hex::encode(received_root),
hex::encode(computed_root),
Expand Down Expand Up @@ -2729,7 +2750,6 @@ mod tests {
#[test]
fn test_c03_pass2_failure_restores_staking_state() {
use sentrix_primitives::block::{Block, STATE_ROOT_FORK_HEIGHT};
use sentrix_primitives::justification::BlockJustification;
use sentrix_staking::staking::ValidatorStake;
use sentrix_storage::MdbxStorage;
use std::sync::Arc;
Expand Down Expand Up @@ -2794,19 +2814,18 @@ mod tests {
let mut block = Block::new(height, prev_hash, vec![coinbase], "v1".into());
block.timestamp = 1_777_000_001;
// Tamper the declared root so #1e fires AFTER the reward bundle ran.
// Leave the block UNJUSTIFIED: a justified #1e block is now accepted
// (canonical-block tolerance), so to still exercise the Pass-2 reject +
// rollback we use the no-justification path, which #1e rejects.
block.state_root = Some([0xAB; 32]);
block.hash = block.calculate_hash();
let mut just = BlockJustification::new(height, 0, block.hash.clone());
just.add_precommit("v1".into(), vec![], 1000);
just.add_precommit("v2".into(), vec![], 1000);
just.add_precommit("v3".into(), vec![], 1000);
block.justification = Some(just);
block.justification = None;

let pending_before = bc.stake_registry.validators.get("v1").unwrap().pending_rewards;

let err = bc
.add_block_from_peer(block)
.expect_err("tampered state_root must be rejected (#1e)");
.expect_err("tampered state_root on an unjustified block must be rejected (#1e)");
assert!(
format!("{err:?}").contains("state_root mismatch"),
"expected #1e state_root mismatch, got: {err:?}"
Expand All @@ -2825,6 +2844,90 @@ mod tests {
}
}

/// A JUSTIFIED Peer block with a tampered (mismatching) state_root past the
/// fork height is ACCEPTED on #1e and the proposer's root stamped —
/// consensus is on block_hash and the block carries a 2/3 justification.
/// This is the lagging-validator catch-up fix (val1 stuck-sync 2026-06-06).
/// The no-justification counterpart staying rejected is covered by
/// `test_c03_pass2_failure_restores_staking_state` above.
#[test]
fn test_justified_peer_block_accepted_on_state_root_mismatch() {
use sentrix_primitives::block::{Block, STATE_ROOT_FORK_HEIGHT};
use sentrix_primitives::justification::BlockJustification;
use sentrix_staking::staking::ValidatorStake;
use sentrix_storage::MdbxStorage;
use std::sync::Arc;
use tempfile::TempDir;

let mut bc = setup();
bc.voyager_activated = true;
for addr in ["v1", "v2", "v3", "v4"] {
bc.stake_registry.validators.insert(
addr.to_string(),
ValidatorStake {
address: addr.to_string(),
self_stake: 1000,
total_delegated: 0,
commission_rate: 1000,
max_commission_rate: 2000,
is_jailed: false,
jail_until: 0,
is_tombstoned: false,
blocks_signed: 0,
blocks_missed: 0,
pending_rewards: 0,
registration_height: 0,
last_commission_change_height: 0,
},
);
}
bc.stake_registry.active_set = vec!["v1".into(), "v2".into(), "v3".into(), "v4".into()];

// Pad past STATE_ROOT_FORK_HEIGHT so the #1e check enforces.
let pad_height = STATE_ROOT_FORK_HEIGHT + 1;
let prev = bc.latest_block().unwrap().hash.clone();
let mut pad = Block::new(
pad_height,
prev,
vec![Transaction::new_coinbase("v1".into(), 0, pad_height, 1_777_000_000)],
"v1".into(),
);
pad.timestamp = 1_777_000_000;
bc.chain.push(pad);

let dir = TempDir::new().unwrap();
let mdbx = Arc::new(MdbxStorage::open(dir.path()).unwrap());
bc.init_trie(mdbx).unwrap();

let height = bc.height() + 1;
let prev_hash = bc.latest_block().unwrap().hash.clone();
let reward = bc.get_block_reward();
let coinbase = Transaction::new_coinbase("v1".into(), reward, height, 1_777_000_002);
let mut block = Block::new(height, prev_hash, vec![coinbase], "v1".into());
block.timestamp = 1_777_000_002;
// Tampered root → forces #1e; justification present → must be accepted.
block.state_root = Some([0xAB; 32]);
block.hash = block.calculate_hash();
let mut just = BlockJustification::new(height, 0, block.hash.clone());
just.add_precommit("v1".into(), vec![], 1000);
just.add_precommit("v2".into(), vec![], 1000);
just.add_precommit("v3".into(), vec![], 1000);
block.justification = Some(just);

let h_before = bc.height();
let result = bc.add_block_from_peer(block);
assert!(
result.is_ok(),
"justified Peer block must be accepted despite #1e: {result:?}"
);
assert_eq!(bc.height(), h_before + 1, "justified block must commit");
assert_eq!(
bc.latest_block().unwrap().state_root,
Some([0xAB; 32]),
"proposer's (received) root must be stamped on #1e accept"
);
}

#[test]
fn test_add_block_succeeds_without_trie() {
// update_trie_for_block returning Ok(None) must not fail add_block.
Expand Down
Loading
Loading