Skip to content

fix(net): validate peer transactions before mempool insertion#73

Open
ace-degen wants to merge 1 commit into
LayerTwo-Labs:mainfrom
ace-degen:fix/p2p-validate-transactions
Open

fix(net): validate peer transactions before mempool insertion#73
ace-degen wants to merge 1 commit into
LayerTwo-Labs:mainfrom
ace-degen:fix/p2p-validate-transactions

Conversation

@ace-degen

Copy link
Copy Markdown

Summary

The P2P NewTransaction handler in lib/node/net_task.rs inserted peer-supplied
transactions into the mempool via MemPool::put without first running
State::validate_transaction. The RPC submission path (Node::submit_transaction)
does validate. Because of this asymmetry, a transaction received from a peer with
an invalid signature (or one that fails balance/fee checks) is accepted into the
mempool and re-broadcast to all peers, even though it can never be included in a
valid block.

Any peer can exploit this to flood the network's mempools with invalid transactions:

  • network-wide mempool poisoning / wasted bandwidth (each node accepts and relays),
  • non-mining nodes never run the template builder, so the junk stays resident in
    their mempools indefinitely (unbounded growth from free, keyless traffic).

(The block-template path is self-healing: Node::get_transactions re-validates and
drops invalid txs before building a template, so they never reach a block — the
impact is relay amplification and mempool bloat, not poisoned templates.)

(Severity: network-level DoS — no direct theft, since invalid transactions are still
rejected at block-connect time.)

Root cause

The handler called only regenerate_proof (which generates a Utreexo proof, not a
validation step) followed by mempool.put (which only guards against double-spends
within the mempool). Neither verifies signatures, address binding, input existence,
or fees.

Fix

Run State::validate_transaction in the NewTransaction handler before
MemPool::put, mirroring the RPC path. One line, inside the existing handler
closure; the ? converts the state::Error via the conversion already used by the
adjacent regenerate_proof call.

Test

Adds lib/mempool.rs::p2p_validation_bypass_tests, a runtime regression test that
funds a real UTXO, builds a transaction spending it but signs with the wrong key,
then asserts:

  1. State::validate_transaction rejects it with an Authorization error, but
  2. the bare regenerate_proof + MemPool::put sequence (what the P2P handler did)
    accepts it and leaves the invalid transaction resident in the mempool.
validate_transaction(forged tx) => Err(Authorization)
test mempool::p2p_validation_bypass_tests::p2p_path_accepts_transaction_that_validation_rejects ... ok
test result: ok. 1 passed; 0 failed

Note

This is one shared bug pattern across the LayerTwo-Labs Rust L2 stack (thunder-rust,
coinshift-rs, photon, plain-bitassets, plain-bitnames, truthcoin-dc); each repo
carries the same gap in its own net_task.rs and gets the same fix.

The P2P `NewTransaction` handler inserted peer-supplied transactions into the
mempool via `MemPool::put` without first running `State::validate_transaction`,
unlike the RPC path (`Node::submit_transaction`). A transaction with an invalid
signature (or failing balance/fee checks) was therefore accepted into the
mempool and re-broadcast to all peers, even though it can never be included in a
valid block. Any peer can use this to flood the network's mempools with invalid
transactions: network-wide relay amplification and mempool bloat (worst on
non-mining nodes, which never run the self-healing template builder). Not theft
— invalid transactions are still rejected at block-connect.

Run `validate_transaction` in the handler before `mempool.put`, matching the RPC
path. Add a regression test pinning the invariant: a forged-signature
transaction is rejected by `validate_transaction` while `MemPool::put` alone
accepts it.
@rsantacroce

Copy link
Copy Markdown
Collaborator

Thanks for the contribution! There are a few CI failures to address — mostly formatting and similar minor issues. Once those are fixed, I'll go ahead and merge.

@rsantacroce

Copy link
Copy Markdown
Collaborator

Thanks for your contribution! There are still a few linting errors to resolve — once those are fixed, I'll get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants