fix: invalidate blocks with invalid transactions, and ban bad/malicious utreexo peers by JoseSK999 · Pull Request #852 · getfloresta/Floresta

JoseSK999 · 2026-02-20T16:36:15Z

Description and Notes

Currently we only handle BlockValidation errors, but validation can also return a TransactionError that must be handled in the same way (both variants wrap BlockValidationErrors).

Also UtxoNotFound must not make us ban the peer sending the block, as the utxos data is provided via utreexo leaf data.

Fixes #851: we don't ban a peer sending us a valid block just because the utreexo leaf data was empty.

How to verify the changes you have done?

When I sync I sometimes find utreexod peers that serve us empty leaf data. Previously this made us ban the peer sending us the block, now we don't. We only see the error we got, and we ban the bad/malicious utreexo peer.

Validation failed for block with Header { block_hash: 00000003a37ac2ed003588e6016371d0bbd15f15095bc01bf790e8fd94efa952, version: Version(536870912), prev_blockhash: 000000018cfd59495300317993f506bc52af3323fd051037e5ac638e5e88e837, merkle_root: 73ea3edcdacf6660ab4cc3ffc7427334f7f23767f17355dc42175fe61f6c45c4, time: 1764953804, bits: CompactTarget(487885650), nonce: 228240630 }, received by peer 0
Reason: TransactionError(TransactionError { txid: 36aa3952cf09b0cf623fa7be9cdceb3187ce110ae97ef47886ad4d9b9ab11dfe, error: UtxoNotFound(OutPoint { txid: b134b1e42b037de72fec05853707fe092bacba47e9733f6d3eff94139a75818e, vout: 2 }) })

EDIT: best to review this PR by commits.

Davidson-Souza · 2026-02-20T17:07:57Z

How about keeping the peer who sent us this proof as well? So we can ban them for sending an invalid proof?

Moreover, if it's not too complicated, we might push the block again the block rather than download it? If we don't ban the peer and drop the block, some rogue peer may make us waste bandwidth by making us re download blocks

JoseSK999 · 2026-02-21T02:16:31Z

Done! Now we ban the utreexo peers that send invalid proofs or incomplete/empty leaf data. I took the opportunity to refactor the error matching/handling logic into handle_validation_errors, which returns the peer to blame about the error: the block peer, leaf peer or proof peer (the last two are now the same utreexo peer, but maybe in the future we can change that, idk).

Also I reduced the REQUEST_TIMEOUT to 1 minute (versus 10 mins). Otherwise I note my node gets stuck if we find one of these utreexo peers to ban.

Below you can see how we don't ban the block peer (id 2) but the utreexo peer (id 11). Then we continue advancing after we retry the utreexo proof requests.

Captura de pantalla 2026-02-21 a las 2 53 29

jaoleal · 2026-02-21T16:36:01Z

LGTM, i just suggest to review the commit structure. I would do 1 commit for the new fields, helpers and separate addition of block proofs and leafs, another for those changes that touch error handling and just one more for reducing the TIMEOUT.

JoseSK999 · 2026-02-22T14:53:56Z

Pushed 5f8a25a

Now InflightBlock has a single aux_data field, currently only for the auxiliary utreexo data (and utreexo peer id). In the future we can make an AuxData enum with UtreexoData and LeafData for SS, so I'm leaving the now unused WireError::LeafDataNotFound.

JoseSK999 · 2026-02-23T14:18:42Z

Pushed d62d7d3

InflightBlock::new adds empty auxiliary utreexo data if the block is empty, simplifying caller logic.
Better comment on aux_data field.
BadMerkleRoot and BadWitnessCommitment now don't invalidate the block, as it may be a mutated txdata and the block is valid given the original txdata.

JoseSK999 · 2026-02-23T16:44:24Z

This failed because now we don't invalidate the BadMerkleRoot block in the test, and SyncNode still makes connections outside our SimulatedPeer list. We were expecting the tip to be at height 6, but real peers on the network give us the true block txdata.

Idk if we should instead add SimulatedPeers as fixed peers to avoid real network peers in wire tests (i.e., we control the test conditions). Wdyt? @Davidson-Souza @moisesPompilio @jaoleal

JoseSK999 · 2026-02-24T19:08:55Z

Check commit d5ac323

I have enforced we don't make real P2P connections in our wire tests, so the SimulatedPeers are the only peers we have. Also changed one test as behavior for BadMerkleRoot errors changed (previously we would just invalidate it)

luisschwab · 2026-02-24T22:23:57Z

Yep, I've been getting this as well:

2026-02-24 19:22:07  INFO floresta_chain::pruned_utreexo::chain_state: New tip! hash=0000001698f9e0807f9db69589c4f3e68c5968ddfcedbea7fd5c39e69748e5de height=2143 tx_count=1
2026-02-24 19:22:07  INFO floresta_chain::pruned_utreexo::chain_state: New tip! hash=0000004162f514768e87545aa9ac3c5d0024aa72871c426817d41dc1b298e94d height=2144 tx_count=1
2026-02-24 19:22:07  INFO floresta_chain::pruned_utreexo::chain_state: New tip! hash=00000080cac5236c3a6d625ab39a874b6b6623d4cf17677e5c719ca9d72cf480 height=2145 tx_count=1
2026-02-24 19:22:07  INFO floresta_chain::pruned_utreexo::chain_state: New tip! hash=00000086ba41e32cbd7255fa75059dca43f24c476df927aef99394c628eaf9d1 height=2146 tx_count=1
2026-02-24 19:22:07  INFO floresta_chain::pruned_utreexo::chain_state: New tip! hash=000000677bba575d2080f42248a45f9f60365125f3223b8c3683a753dd0db2ce height=2147 tx_count=1
2026-02-24 19:22:07 ERROR floresta_wire::p2p_wire::node::blocks: Invalid block Header { block_hash: 0000011c906d0dba02a8c3a718da0eea459f5e14d5759461a6c5af819cf720eb, version: Version(536870912), prev_blockhash: 000000677bba575d2080f42248a45f9f60365125f3223b8c3683a753dd0db2ce, merkle_root: f49bb2d907f5a63f71c1ac5416f69f2cfdf0d09239ebc22f4bb014f7933e9549, time: 1599409399, bits: CompactTarget(503394215), nonce: 4626542 } received by peer 0 reason: TransactionError(TransactionError { txid: 26c444de24f3881d8d88353b7a49912e9f0b5278d3e2145f0195f2f9ec07fb90, error: UtxoNotFound(OutPoint { txid: bec74e808c906f43673ecd13d5e61a89ea2829e959b0042c85726a2e1308f3d1, vout: 0 }) })
2026-02-24 19:22:07  WARN floresta_wire::p2p_wire::node::blocks: Block 0000011c906d0dba02a8c3a718da0eea459f5e14d5759461a6c5af819cf720eb from peer 0 is invalid, banning peer

Davidson-Souza

small nits

JoseSK999 · 2026-03-02T20:38:43Z

Moved new block debug before te early return

jaoleal · 2026-03-02T21:39:59Z

+        // Add the proof and leaf data, together with the peer id that sent them
+        block.add_utreexo_data(uproof.leaf_data, proof, peer);


Question: If a malicious utreexo peer send invalid proofs for a Inflight block we will override the proof correct ?

If the above is true, a node thats slowly processing inflight blocks can be vulnerable right ?

Good point because I think we can accept a UtreexoProof from a peer we didn't ask to @Davidson-Souza

But we will ban that utreexo peer if the data is invalid with the changes in this PR, after processing the block and proof. It's still weird if Alice can send a proof or block that we had requested to Bob

We can follow-up by checking whether the peer who sent us some data is actually the peer we requested to

JoseSK999 · 2026-03-03T01:26:20Z

Pushed 88f7e2b

Updated docstrings and some style changes as suggested by @jaoleal
Allowed the node to connect UTREEXO peers even if we have a fixed peer, if we really need it

Now `InflightBlock` also keeps the peer id that sent the utreexo aux data.

Currently we only handle `BlockValidation` errors, but validation can also return a `TransactionError` that must be handled in the same way (both variants wrap `BlockValidationErrors`). This implements `block_validation_err` helper so that we also handle `TransactionError` and others.

JoseSK999 · 2026-03-03T14:21:44Z

Pushed 2990d74

Added the true utreexo_peer to the UtreexoProof inflight request

Davidson-Souza · 2026-03-03T16:16:09Z

+        let (peer_id, address) = match &self.fixed_peer {
+            // We have a fixed peer, and we don't need more/any utreexo peers
+            Some(addr) if !needs_utreexo || self.has_utreexo_peers() => (0, addr.clone()),
+
+            // No fixed peer or a non-utreexo fixed peer: try to connect to P2P utreexo ones
+            _ => self
+                .address_man
+                .get_address_to_connect(services, feeler)
+                // No peers with the desired services are known, load hardcoded addresses
+                .ok_or_else(|| {
+                    let net = self.network;
+                    self.address_man.add_fixed_addresses(net);
+                    WireError::NoAddressesAvailable
+                })?,


So, if we don't have a utreexo fixed peer, we create a connection to any other peer? I think this is wrong.

I would just change this function to ignore the passed in service bits if we are using a fixed peer

If we don't have a utreexo fixed peer, but normal fixed peer, we try network peers iff:

needs_utreexo && !self.has_utreexo_peers()

Wasn't your concern that we would not create UTREEXO connections if we have a normal fixed peer?

No, my concern was that we would create a connection and then disconnect it "due to not having the required services". I think we need to honor the user's choice to connect with that node, even if it doesn't support utreexo

Oh I misunderstood you then. But I don't think that issue would happen because we are not only trying UTREEXO connections, we also try NETWORK or NONE in ChainSelector/RunningNode. No?

I agree we can make the connection kind NONE if there's a fixed peer tho

Nise, reverting last commit changes wrt fixed peer

Someone needs to take over 636

@Davidson-Souza I think if you use a fixed peer you will anyway be stuck at ChainSelector 😂 TODO fix in a followup

Someone needs to take over 636

We should ask if he want to at least finish the opened ones. I vote to make it priority for this release, and come before #865 (it makes fixing the bug mentioned there much easier).

@Davidson-Souza I think if you use a fixed peer you will anyway be stuck at ChainSelector 😂 TODO fix in a followup

Why? Because of the 10 peers minimum? We already account for that in can_start_headers_sync

/// Whether we have enough peers to start downloading headers fn can_start_headers_sync(&self) -> bool { let connected_peers = self.connected_peers(); if self.network == Network::Regtest && connected_peers >= 1 { return true; } if self.fixed_peer.is_some() && connected_peers >= 1 { return true; } connected_peers >= ChainSelector::MAX_OUTGOING_PEERS }

We already account for that in can_start_headers_sync

Oh I forgot, true!

This new methods matches against all block validation errors that we can get and returns which peer is the one to blame. The only new behavior is that `InvalidProof` and `UtxoNotFound` return the utreexo peer so we can disconnect and ban it (using a new `disconnect_and_ban` method), and an invalid txid/wtxid merkle root doesn't invalidate the block, since it may be mutated data. Another change in behavior is that we don't ban any peer if there's no error related to validation, like a database error. In the future we must figure out how to handle these other possible errors.

Also `test_sync_invalid_block` was actually testing a mutated block, but now we don't invalidate it: we keep waiting to find the matching txdata or a better chain. Thus `setup_node` wouldn't exit as IBD was unfinished. Instead, the test now exercises the case where 9 peers send a mutated block and one the true block.

JoseSK999 · 2026-03-03T17:10:09Z

Pushed 8e30d6e

Reverted changes in create_connection related to the fixed peer
Removed newline in info!
Removed one ServiceFlags::from(1 << 25)

Davidson-Souza

ACK 8e30d6e

jaoleal · 2026-03-04T19:30:03Z

ACK 8e30d6e

6de9fa1 feat(wire): make sure we don't ban Manual connections (Davidson Souza) 31c21a5 feat(wire): add a `Manual` variant to PeersKind (Davidson Souza) Pull request description: ### Description and Notes All peers that are added by the user — those added with the RPC `addnode` or the CLI option `--connect`, should have kind `Manual`, rather than `Regular`. This exempts them from: - Max peers quota — if we already have 10 peers and the user tries to addnode another one, we will now have 11 peers - They whitelisted and shouldn't be banned by misbehaving - They don't need to have any required service This commit adds this new variant, and makes sure we are following the above rules. This is a small subset of #636, and will be useful for getting #865 unstuck (potentially can help with some edge cases as mentioned in #852 (comment)) ACKs for top commit: jaoleal: ACK 6de9fa1 JoseSK999: re-ACK 6de9fa1 Tree-SHA512: 5d620318551ba4078fcc66d1f6af380e9875293f10e9c09e649f0f06f532c62df30b4415c3dd304eebd88039369eae96ec1f2e57ed7fb174fe7d042a5e6bf6f7

JoseSK999 added the bug Something isn't working label Feb 20, 2026

moisesPompilio assigned JoseSK999 and unassigned JoseSK999 Feb 20, 2026

JoseSK999 force-pushed the handle-transaction-errors branch from 9e90e0e to 6e661e4 Compare February 21, 2026 01:55

JoseSK999 changed the title ~~fix: invalidate blocks with invalid transactions~~ fix: invalidate blocks with invalid transactions, and ban bad/malicious utreexo peers Feb 21, 2026

JoseSK999 mentioned this pull request Feb 21, 2026

0.9.0 priorities #850

Closed

20 tasks

jaoleal reviewed Feb 21, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/node/blocks.rs Outdated

jaoleal reviewed Feb 21, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/node/blocks.rs

Davidson-Souza added this to the v0.9.0 milestone Feb 22, 2026

JoseSK999 force-pushed the handle-transaction-errors branch from 6e661e4 to 5f8a25a Compare February 22, 2026 14:44

Davidson-Souza requested changes Feb 22, 2026

View reviewed changes

JoseSK999 force-pushed the handle-transaction-errors branch from 5f8a25a to d62d7d3 Compare February 23, 2026 14:11

JoseSK999 force-pushed the handle-transaction-errors branch from d62d7d3 to d5ac323 Compare February 24, 2026 19:05

luisschwab self-requested a review February 26, 2026 15:06

Davidson-Souza reviewed Mar 2, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/node/blocks.rs Outdated

Comment thread crates/floresta-wire/src/p2p_wire/node/conn.rs

Comment thread crates/floresta-wire/src/p2p_wire/tests/sync_node.rs

JoseSK999 force-pushed the handle-transaction-errors branch from d5ac323 to deed943 Compare March 2, 2026 20:28

jaoleal reviewed Mar 2, 2026

View reviewed changes

JoseSK999 force-pushed the handle-transaction-errors branch from deed943 to 88f7e2b Compare March 3, 2026 01:18

Davidson-Souza reviewed Mar 3, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/node/blocks.rs

chore(wire): add InflightBlock auxiliary data

53de82e

Now `InflightBlock` also keeps the peer id that sent the utreexo aux data.

JoseSK999 force-pushed the handle-transaction-errors branch from 88f7e2b to 2990d74 Compare March 3, 2026 14:16

Davidson-Souza reviewed Mar 3, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/node/blocks.rs Outdated

Davidson-Souza reviewed Mar 3, 2026

View reviewed changes

Comment thread crates/floresta-wire/src/p2p_wire/tests/utils.rs Outdated

Davidson-Souza reviewed Mar 3, 2026

View reviewed changes

JoseSK999 added 5 commits March 3, 2026 17:52

chore(chain): remove unused error field

e9f6d45

chore(wire): set SyncNode::REQUEST_TIMEOUT to 1 min

8b84b40

chore(wire): populate services for mock peers

8e30d6e

JoseSK999 force-pushed the handle-transaction-errors branch from 2990d74 to 8e30d6e Compare March 3, 2026 17:04

Davidson-Souza approved these changes Mar 3, 2026

View reviewed changes

Davidson-Souza mentioned this pull request Mar 3, 2026

Feat: add Manual type to ConnectionKinds #866

Merged

JoseSK999 requested a review from jaoleal March 4, 2026 12:31

Davidson-Souza merged commit 6242d2f into getfloresta:master Mar 4, 2026
11 checks passed

JoseSK999 deleted the handle-transaction-errors branch March 4, 2026 20:42

csgui added this to Floresta Mar 12, 2026

github-project-automation Bot moved this to Backlog in Floresta Mar 12, 2026

csgui moved this from Backlog to Done in Floresta Mar 12, 2026

		// Add the proof and leaf data, together with the peer id that sent them
		block.add_utreexo_data(uproof.leaf_data, proof, peer);

Uh oh!

Conversation

JoseSK999 commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and Notes

How to verify the changes you have done?

Uh oh!

Davidson-Souza commented Feb 20, 2026

Uh oh!

JoseSK999 commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaoleal commented Feb 21, 2026

Uh oh!

JoseSK999 commented Feb 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoseSK999 commented Feb 23, 2026

Uh oh!

JoseSK999 commented Feb 23, 2026

Uh oh!

JoseSK999 commented Feb 24, 2026

Uh oh!

luisschwab commented Feb 24, 2026

Uh oh!

Davidson-Souza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoseSK999 commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoseSK999 commented Mar 3, 2026

Uh oh!

Uh oh!

JoseSK999 commented Mar 3, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoseSK999 Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoseSK999 Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JoseSK999 commented Feb 20, 2026 •

edited

Loading

JoseSK999 commented Feb 21, 2026 •

edited

Loading

JoseSK999 Mar 3, 2026 •

edited

Loading

JoseSK999 Mar 3, 2026 •

edited

Loading