Fix some public security issues

[Officeyutong]

What problem does this PR solve?

This PR will fix the following public security issues:

• An overflow in consensus.rs which will happen after 256 years
• Two overflows in peer_store_impl & net.rs set_ban. There overflows can be triggered by passing specific arguments, but has little impact.
• Compact-block relay accepted headers as valid before checking the consensus epoch difficulty, allowing low-difficulty invalid compact blocks to poison relay state and trigger reconstruction requests.
• Identify accepted untrusted DNS listen addresses into the peer store, allowing malicious peers to influence later outbound dialing targets.
• RecentReject failed to update its in-memory key count after writes, allowing remote rejected transactions to bypass the configured count limit and grow the reject database.
• ConnectionRequest trusted an unauthenticated from peer id, allowing forged hole-punching state to trigger outbound NAT traversal attempts to attacker-controlled addresses.
• HeaderVerifier failed to enforce the consensus block header version, allowing nonzero-version blocks to be accepted through some ingress paths while SendHeaders rejected them.
• ShortIdsVerifier failed to bound compact-block short IDs, allowing oversized compact blocks to amplify relay reconstruction work and pending-state growth.

What is changed and how it works?

What's Changed:

Related changes

• PR to update owner/repo:
• Need to cherry-pick to the release branch

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

• Performance regression
• Breaking backward compatibility

[chenyukang]

This still skips the epoch/compact-target validation when the parent is known only through header_map. The earlier get_header_index_view(..., store_first) path accepts such parents, but this new check only looks in the block store; in that case an invalid compact block can still pass contextual_check and enter pending/request missing transactions. Please validate against the parent header already accepted above, or reject when the expected epoch cannot be derived, and add a regression for a header-map parent with a wrong compact target.

[Officeyutong]

Fixed

[chenyukang]

Checking the session peer only when route is empty leaves a forged-from bypass: a direct peer can send a ConnectionRequest with a non-empty, unauthenticated route and an arbitrary from, and this branch will skip the binding check. The direct sender should be authenticated as route.last().unwrap_or(from) (or non-empty externally supplied routes should be rejected unless the last hop matches the session peer). Please add a regression for forged from with a non-empty route.

[Officeyutong]

Fixed

[chenyukang]

This rejects all remote listen addresses that cannot be converted to a SocketAddr, which also drops /onion3 addresses. The identify protocol still explicitly allows advertising local /onion3 listen addresses below, and the peer store has onion-specific handling, so this looks like a compatibility regression while fixing DNS loopback injection. Please keep rejecting unsafe DNS addresses without filtering out the existing onion address support.

[Officeyutong]

Fixed. Now /onion3 procotol is explicitly allowed.

[eval-exec]

How about this for better readability:

self.total_keys_num = self.total_keys_num.saturating_add(1);

if self.total_keys_num > self.count_limit {
    self.shrink()?;
}

[Officeyutong]

Fixed

[Copilot]

Pull request overview

This PR hardens several consensus, networking, sync-relay, and RPC edge cases to prevent malformed inputs from causing overflows, state poisoning, or unbounded resource growth, aligning multiple ingress paths with consensus validation.

Changes:

• Enforce additional header / compact-block validity checks (header version, compact-block short-id bounds, and compact-block epoch/target validation before pending-state effects).
• Fix multiple overflow paths and resource-limit enforcement (epoch reward halving shift, peer-store banning timestamps, RPC set_ban, and tx-pool RecentReject key-count tracking).
• Tighten trust boundaries for peer-provided data (reject unauthenticated hole-punching from peer id, and filter untrusted identify listen addrs).

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
verification/src/tests/header_verifier.rs	Adds a test ensuring non-consensus header versions are rejected.
verification/src/header_verifier.rs	Enforces consensus header version in HeaderVerifier.
tx-pool/src/component/tests/recent_reject.rs	Adds regression test for RecentReject count-limit enforcement after writes.
tx-pool/src/component/recent_reject.rs	Fixes in-memory key count tracking after successful put.
sync/src/relayer/tests/helper.rs	Implements mock filter broadcast paths to avoid unimplemented!() panics in tests.
sync/src/relayer/tests/compact_block_verifier.rs	Adds test for short-id count upper bound.
sync/src/relayer/tests/compact_block_process.rs	Adds tests for compact-block parent/header-map acceptance and invalid-difficulty behavior.
sync/src/relayer/compact_block_verifier.rs	Rejects compact blocks with too many short IDs to bound relay work.
sync/src/relayer/compact_block_process.rs	Adds epoch/target consistency check for compact-block headers (when parent is in store).
spec/src/consensus.rs	Avoids invalid right-shifts in epoch reward halving by clamping to zero after many halvings.
rpc/src/tests/setup.rs	Ensures RPC tests build a global runtime and wire async handle into SharedBuilder.
rpc/src/tests/module/test.rs	Refactors TCP RPC test call style (no behavior change).
rpc/src/tests/module/net.rs	Adds tests covering set_ban relative-time overflow handling.
rpc/src/tests/module/mod.rs	Registers the new net test module.
rpc/src/tests/mod.rs	Extends RpcTestSuite to hold runtime lifecycle fields.
rpc/src/module/net.rs	Uses checked addition for relative ban_time and returns InvalidParams on overflow.
network/src/tests/peer_store.rs	Adds regression test ensuring ban timestamp computation saturates rather than overflows.
network/src/protocols/tests/mod.rs	Adds identify test for rejecting DNS loopback listen addresses.
network/src/protocols/identify/mod.rs	Introduces is_remote_listen_addr_allowed and uses it to filter remote listen addrs.
network/src/protocols/hole_punching/component/connection_request.rs	Validates from peer id against authenticated session peer id for direct (no-route) requests + adds test.
network/src/peer_store/peer_store_impl.rs	Prevents ban-until overflow via saturating_add.

---

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

[chainTe]

Security review finding:

The compact-block epoch/compact-target guard added in contextual_check only runs when the parent is already stored in the chain DB:

if let Some(parent) = shared.store().get_block_header(&parent_hash) {
    ... compare expected_epoch / expected_compact_target ...
}

But this same function accepts parents from shared.get_header_index_view(...), which can come from header_map when the parent header was received but the block is not stored yet. In that case the new guard is skipped entirely. The ordinary HeaderVerifier only checks PoW, number, continuous epoch, and timestamp; it does not verify the exact consensus epoch fraction or compact target. After this bypass, the compact-block path calls insert_valid_header, then on missing transactions stores the compact block in pending_compact_blocks and sends GetBlockTransactions.

A peer can therefore send a header-map parent and then a child compact block with an invalid compact target/epoch that still passes the lighter header checks, causing the node to retain/request data for an invalid compact block until later full-block validation. This appears to leave the compact-block DoS fix incomplete for non-stored parents.

Suggested fix: resolve the parent through the same source that made parent.is_some() true, and perform the exact epoch/compact-target validation for header-map parents too, or require this check before queuing/requesting missing compact-block data. Please also add a test variant of test_compact_block_accepts_parent_from_header_map where the child has an invalid compact target/epoch and assert it is rejected and not inserted into pending_compact_blocks.

[Officeyutong]

Security review finding:

The compact-block epoch/compact-target guard added in contextual_check only runs when the parent is already stored in the chain DB:

if let Some(parent) = shared.store().get_block_header(&parent_hash) {
    ... compare expected_epoch / expected_compact_target ...
}

But this same function accepts parents from shared.get_header_index_view(...), which can come from header_map when the parent header was received but the block is not stored yet. In that case the new guard is skipped entirely. The ordinary HeaderVerifier only checks PoW, number, continuous epoch, and timestamp; it does not verify the exact consensus epoch fraction or compact target. After this bypass, the compact-block path calls insert_valid_header, then on missing transactions stores the compact block in pending_compact_blocks and sends GetBlockTransactions.

A peer can therefore send a header-map parent and then a child compact block with an invalid compact target/epoch that still passes the lighter header checks, causing the node to retain/request data for an invalid compact block until later full-block validation. This appears to leave the compact-block DoS fix incomplete for non-stored parents.

Suggested fix: resolve the parent through the same source that made parent.is_some() true, and perform the exact epoch/compact-target validation for header-map parents too, or require this check before queuing/requesting missing compact-block data. Please also add a test variant of test_compact_block_accepts_parent_from_header_map where the child has an invalid compact target/epoch and assert it is rejected and not inserted into pending_compact_blocks.

This is already addressed in the current branch. contextual_check now validates the compact block against the parent resolved by get_header_index_view, including header-map-only parents, and rejects the compact block if the expected epoch cannot be derived. I also added test_invalid_compact_target_with_parent_from_header_map_does_not_enter_pending to cover the non-stored parent case and assert it does not enter pending or send GetBlockTransactions.

[chenyukang]

it's better to return temporary error here?

diff --git a/sync/src/relayer/compact_block_process.rs b/sync/src/relayer/compact_block_process.rs
index d03794c0c..ce784f3b5 100644
--- a/sync/src/relayer/compact_block_process.rs
+++ b/sync/src/relayer/compact_block_process.rs
@@ -358,10 +358,7 @@ async fn contextual_check(
     };
 
     let Some(epoch) = maybe_epoch else {
-        shared
-            .shared()
-            .insert_block_status(block_hash.clone(), BlockStatus::BLOCK_INVALID);
-        return StatusCode::CompactBlockHasInvalidHeader.with_context(format!(
+        return StatusCode::CompactBlockRequiresParent.with_context(format!(
             "{block_hash} failed to derive expected epoch from parent {parent_hash}"
         ));
     };
diff --git a/sync/src/relayer/tests/compact_block_process.rs b/sync/src/relayer/tests/compact_block_process.rs
index 6421fbd29..64447edfd 100644
--- a/sync/src/relayer/tests/compact_block_process.rs
+++ b/sync/src/relayer/tests/compact_block_process.rs
@@ -47,6 +47,27 @@ fn new_non_tail_header_map_parent(relayer: &crate::relayer::Relayer) -> HeaderVi
     panic!("failed to build a header-map parent with derivable next epoch");
 }
 
+fn new_tail_header_map_parent(relayer: &crate::relayer::Relayer) -> HeaderView {
+    let shared = relayer.shared.shared();
+    let tip_number = relayer.shared.active_chain().tip_header().number();
+
+    for number in (0..=tip_number).rev() {
+        let stored_parent = shared
+            .store()
+            .get_block_hash(number)
+            .and_then(|block_hash| shared.store().get_block_header(&block_hash))
+            .unwrap();
+        let header = new_header_builder(shared, &stored_parent).build();
+        let epoch = header.epoch();
+
+        if epoch.index() + 1 == epoch.length() {
+            return header;
+        }
+    }
+
+    panic!("failed to build a tail header-map parent");
+}
+
 fn child_epoch_after(parent_epoch: EpochNumberWithFraction) -> EpochNumberWithFraction {
     assert!(parent_epoch.index() + 1 < parent_epoch.length());
     EpochNumberWithFraction::new(
@@ -563,6 +584,93 @@ fn test_invalid_compact_target_with_parent_from_header_map_does_not_enter_pendin
     assert!(!nc.has_sent(SupportProtocols::RelayV3.protocol_id(), peer_index, data));
 }
 
+#[test]
+fn test_compact_block_with_tail_parent_from_header_map_does_not_mark_invalid() {
+    let (_chain, relayer, _) = build_chain(20);
+    let header_map_parent = new_tail_header_map_parent(&relayer);
+    let parent_epoch = header_map_parent.epoch();
+    let child_epoch =
+        EpochNumberWithFraction::new(parent_epoch.number() + 1, 0, parent_epoch.length());
+    let child_header = HeaderBuilder::default()
+        .parent_hash(header_map_parent.hash())
+        .number(header_map_parent.number() + 1)
+        .timestamp(header_map_parent.timestamp() + 1)
+        .epoch(child_epoch)
+        .compact_target(header_map_parent.compact_target())
+        .build();
+
+    let rt = tokio::runtime::Builder::new_current_thread()
+        .enable_all()
+        .build()
+        .unwrap();
+    let peer_index: PeerIndex = 100.into();
+    relayer
+        .shared()
+        .insert_valid_header(peer_index, &header_map_parent);
+    assert!(
+        relayer
+            .shared()
+            .store()
+            .get_block_header(&header_map_parent.hash())
+            .is_none()
+    );
+
+    let block = BlockBuilder::default()
+        .header(child_header)
+        .transaction(TransactionBuilder::default().build())
+        .transaction(
+            TransactionBuilder::default()
+                .output(
+                    CellOutputBuilder::default()
+                        .capacity(Capacity::bytes(1).unwrap())
+                        .build(),
+                )
+                .output_data(Bytes::new())
+                .build(),
+        )
+        .build();
+    let block_hash = block.header().hash();
+
+    let mut prefilled_transactions_indexes = HashSet::new();
+    prefilled_transactions_indexes.insert(0);
+    let compact_block = CompactBlock::build_from_block(&block, &prefilled_transactions_indexes);
+
+    let mock_protocol_context = MockProtocolContext::new(SupportProtocols::RelayV3);
+    let nc = Arc::new(mock_protocol_context);
+    let compact_block_process = CompactBlockProcess::new(
+        compact_block.as_reader(),
+        &relayer,
+        Arc::<MockProtocolContext>::clone(&nc),
+        peer_index,
+    );
+
+    assert_eq!(
+        rt.block_on(compact_block_process.execute()),
+        StatusCode::CompactBlockRequiresParent.into()
+    );
+    assert_ne!(
+        relayer
+            .shared()
+            .active_chain()
+            .get_block_status(&block_hash),
+        BlockStatus::BLOCK_INVALID
+    );
+
+    let pending_compact_blocks = rt.block_on(relayer.shared.state().pending_compact_blocks());
+    assert!(pending_compact_blocks.get(&block_hash).is_none());
+
+    let content = packed::GetBlockTransactions::new_builder()
+        .block_hash(block_hash)
+        .indexes([1u32])
+        .uncle_indexes(Vec::<u32>::new())
+        .build();
+    let message = packed::RelayMessage::new_builder().set(content).build();
+    let data = message.as_bytes();
+
+    std::thread::sleep(std::time::Duration::from_millis(100));
+    assert!(!nc.has_sent(SupportProtocols::RelayV3.protocol_id(), peer_index, data));
+}
+
 #[test]
 fn test_invalid_difficulty_compact_block_does_not_enter_pending() {
     let (_chain, relayer, _) = build_chain(5);

[Officeyutong]

Now the block won't be marked as invalid

[gpBlockchain]

Security review completed for PR #5219 at head a9fbf4b against base 20d76a9. I found no reportable security issues in the reviewed diff. Targeted checks covered the P2P hole-punching/identify changes, compact-block validation, header version validation, reward halving behavior, peer ban overflow handling, and tx-pool recent-reject accounting. The RPC set_ban regression tests were attempted but remained blocked by an RPC test-harness EOF during response decode before the method assertions ran, so CI or a known-good RPC test environment should still cover that path.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

[chenyukang]

@gpBlockchain wrote:
Security review completed for PR #5219 at head a9fbf4b against base 20d76a9. I found no reportable security issues in the reviewed diff. Targeted checks covered the P2P hole-punching/identify changes, compact-block validation, header version validation, reward halving behavior, peer ban overflow handling, and tx-pool recent-reject accounting. The RPC set_ban regression tests were attempted but remained blocked by an RPC test-harness EOF during response decode before the method assertions ran, so CI or a known-good RPC test environment should still cover that path.

This PR will be merged after security release