Skip to content

chore: Improve P2P & mempool robustness and reorg handling#280

Merged
BlobMaster41 merged 6 commits intomainfrom
fix/patch-bugs
Apr 10, 2026
Merged

chore: Improve P2P & mempool robustness and reorg handling#280
BlobMaster41 merged 6 commits intomainfrom
fix/patch-bugs

Conversation

@BlobMaster41
Copy link
Copy Markdown
Contributor

@BlobMaster41 BlobMaster41 commented Apr 8, 2026

Description

Multiple robustness and correctness fixes across networking and mempool code:

  • BitcoinTransactionVerificatorV2: Harden onBlockChange by adding target/current height tracking, non-poisoning queue (separate caller-visible promise and trailing .catch), retries with exponential backoff, abortable sleep to cancel stale retries, and staleness checks to avoid applying outdated solutions. Adds tests covering retries, abandonment, reorgs, and queue health.

  • Mempool.watchBlockchain: Track latestObservedHeight to avoid stale callbacks and compute fullSync against Bitcoin Core tip (with 1-block tolerance) instead of a tautological diff. Adds tests for fullSync behavior.

  • P2PConfigurations: Replace hardcoded noAnnounce list with announceFilter using isPrivate to filter private addresses.

  • AddressExtractor: Correct protocol parsing by treating valued protocols consistently (avoid mis-parsing components without values).

  • P2PManager & PeerChecker: Improve blacklist/unreachable handling (avoid duplicate processing), increase blacklist purge timeout, add isMonitoringHealth guard, skip unreachable peers when dialing/re-adding, better logging, and ensure external dial failures are recorded. Small fixes to event handlers and address filtering.

  • BlockWitnessManager: Adjust reorg check from >= to > to avoid incorrect reorg handling when equal.

Overall this commit fixes race conditions, prevents internal queues from being poisoned by transient failures, improves reorg correctness, and tightens P2P peer/address handling. Tests were added for core mempool/verificator behaviours.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Performance improvement
  • Consensus change (changes that affect state calculation or validation)
  • Refactoring (no functional changes)
  • Documentation update
  • CI/CD changes
  • Dependencies update

Checklist

Build & Tests

  • npm install completes without errors
  • npm run build completes without errors
  • npm test passes all tests

Code Quality

  • Code follows the project's coding standards
  • No new compiler warnings introduced
  • Error handling is appropriate
  • Logging is appropriate for debugging and monitoring

Documentation

  • Code comments added for complex logic
  • Public APIs are documented
  • README updated (if applicable)

Security

  • No sensitive data (keys, credentials) committed
  • No new security vulnerabilities introduced
  • RPC endpoints properly authenticated
  • Input validation in place for external data

OP_NET Node Specific

  • Changes are compatible with existing network state
  • Consensus logic changes are documented and tested
  • State transitions are deterministic
  • WASM VM execution is reproducible across nodes
  • P2P protocol changes are backward-compatible (or migration planned)
  • Database schema changes include migration path
  • Epoch finality and PoC/PoW logic unchanged (or documented if changed)

Testing

Consensus Impact

Related Issues


By submitting this PR, I confirm that my contribution is made under the terms of the project's license.

@BlobMaster41 BlobMaster41 added bug Something isn't working enhancement New feature or request core component fix audit labels Apr 8, 2026
Drop the unused `currentSolutionsHeight` field and its assignment in the challenge-loading flow. Add an explicit `e: unknown` type on a catch handler to avoid implicit any, and apply minor import/whitespace reformatting and a small comment tweak. No functional behavior changes intended.

Always refetch challenges; update tests

Bump README to v1.0.3 and clarify refetch behavior in BitcoinTransactionVerificatorV2: block height is not a safe cache key under reorgs, so always refetch when onBlockChange is called and rely on targetSolutionsHeight to collapse in-flight duplicate requests. Tests updated to remove reliance on the private currentSolutionsHeight field, asserting against allowedChallenges behavior instead (including failure/retry semantics and reorg scenarios), and to capture the initial promise where appropriate.

Improve P2P & mempool robustness and reorg handling

Multiple robustness and correctness fixes across networking and mempool code:

- BitcoinTransactionVerificatorV2: Harden onBlockChange by adding target/current height tracking, non-poisoning queue (separate caller-visible promise and trailing .catch), retries with exponential backoff, abortable sleep to cancel stale retries, and staleness checks to avoid applying outdated solutions. Adds tests covering retries, abandonment, reorgs, and queue health.

- Mempool.watchBlockchain: Track latestObservedHeight to avoid stale callbacks and compute fullSync against Bitcoin Core tip (with 1-block tolerance) instead of a tautological diff. Adds tests for fullSync behavior.

- P2PConfigurations: Replace hardcoded noAnnounce list with announceFilter using isPrivate to filter private addresses.

- AddressExtractor: Correct protocol parsing by treating valued protocols consistently (avoid mis-parsing components without values).

- P2PManager & PeerChecker: Improve blacklist/unreachable handling (avoid duplicate processing), increase blacklist purge timeout, add isMonitoringHealth guard, skip unreachable peers when dialing/re-adding, better logging, and ensure external dial failures are recorded. Small fixes to event handlers and address filtering.

- BlockWitnessManager: Adjust reorg check from >= to > to avoid incorrect reorg handling when equal.

Overall this commit fixes race conditions, prevents internal queues from being poisoned by transient failures, improves reorg correctness, and tightens P2P peer/address handling. Tests were added for core mempool/verificator behaviours.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves robustness and correctness across the P2P layer and mempool/block-change handling, with added regression tests targeting prior audit findings and reorg/race edge cases.

Changes:

  • Harden mempool/block-change processing against races and transient failures (staleness tracking, retries/backoff, non-poisoning queues).
  • Improve P2P peer handling (unreachable/blacklist behavior, connection-health monitoring guards, address announcement filtering).
  • Add extensive tests covering mempool/verificator behavior and reorg/watchdog race scenarios; bump README version.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/witness/WitnessThread.test.ts Minor test formatting/expectation layout tweaks.
tests/vm/deployContractAtAddress.test.ts Test formatting refactors; preserves existing behavior.
tests/reorg/watchdog/toctouCurrentHeader.test.ts Reformats async expect blocks; minor comment formatting.
tests/reorg/watchdog/doubleRevertRace.test.ts Reformats spies/expect blocks; minor comment formatting.
tests/reorg/blockindexer/stuckFlagRecovery.test.ts Reformats calls/spies; minor comment formatting.
tests/mempool/MempoolWatchBlockchain.test.ts New tests validating watchBlockchain fullSync/staleness behavior.
tests/mempool/BitcoinTransactionVerificatorV2.test.ts New tests validating retry/backoff, staleness abandonment, and queue health.
src/src/poc/networking/PeerChecker.ts Avoid duplicate “unreachable” handling; centralize failure tracking for external dials.
src/src/poc/networking/P2PManager.ts Improve unreachable/blacklist handling, prevent duplicate processing, guard health monitoring, skip unreachable peers.
src/src/poc/networking/p2p/BlockWitnessManager.ts Fix reorg boundary condition by changing >= to >.
src/src/poc/networking/AddressExtractor.ts Fix multiaddr component parsing by distinguishing valued vs valueless protocols.
src/src/poc/mempool/verificator/bitcoin/v2/BitcoinTransactionVerificatorV2.ts Add non-poisoning queue, target-height tracking, retries w/ exponential backoff, and staleness checks.
src/src/poc/mempool/manager/Mempool.ts Track latest observed height and compute fullSync vs Bitcoin Core tip with staleness guards.
src/src/poc/configurations/P2PConfigurations.ts Replace hardcoded noAnnounce with announceFilter using isPrivate.
README.md Version bump to v1.0.3.

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

Comment thread src/src/poc/configurations/P2PConfigurations.ts Outdated
Comment thread src/src/poc/mempool/manager/Mempool.ts
Comment thread src/src/poc/networking/AddressExtractor.ts Outdated
@BlobMaster41 BlobMaster41 changed the title Improve P2P & mempool robustness and reorg handling chore: Improve P2P & mempool robustness and reorg handling Apr 9, 2026
Reorganize P2PConfigurations imports and apply a minor stylistic tweak; remove an unused TRANSPORT_PROTOCOLS constant.

- P2PConfigurations.ts: reorder multiaddr/Multiaddr imports and add parentheses around the announceFilter arrow parameter for consistency.
- AddressExtractor.ts: delete the unused TRANSPORT_PROTOCOLS set.

These are small cleanup changes to improve code style and remove dead code.

Update BitcoinTransactionVerificatorV2.ts

Add recommended Bitcoin mainnet node setup

Adds a "Recommended Bitcoin Mainnet Node Setup" section to the README with instructions to install Bitcoin Core and a sample bitcoin.conf. The snippet includes RPC and node settings (rpcuser, rpcpassword, server, txindex, prune, datadir, rpcport, rpcworkqueue, rpcthreads, timeouts, mempool settings, etc.) and notes to adjust rpcuser, rpcpassword, and datadir for your environment.
Introduce a patch-aware parsing flow for epoch graffiti: add a patchHeight constant and use readBytesWithLength for blocks before the patch (preserving v1.0.1 parse behavior and keeping resync consistency), while using raw readBytes for blocks at/after the patch. Keeps the max-length check and throws an error when graffiti is too long.
Reorder the declaration of GRAFFITI_LENGTH_PATCH_BLOCK_HEIGHT so patchHeight is defined immediately after maxLength and before the graffiti variable. This groups consensus-related constants together for clarity; no functional behavior is changed.
Add handling for legacy graffiti encoding in DeploymentTransaction: import BinaryWriter and Submission, and introduce getGraffiti(submission) which conditionally length-prefixes graffiti for blocks before OPNetConsensus.consensusEpochPatches.GRAFFITI_LENGTH_PATCH_BLOCK_HEIGHT. Use getGraffiti when constructing deployment witness data to preserve backward-compatible serialization of graffiti across the consensus patch.
@BlobMaster41 BlobMaster41 merged commit 5fdf0b6 into main Apr 10, 2026
11 checks passed
@BlobMaster41 BlobMaster41 deleted the fix/patch-bugs branch April 10, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit bug Something isn't working core component enhancement New feature or request fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants