p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

glozow · 2024-04-29T20:59:09Z

Part of #27463 in the "make orphan handling more robust" section.

Currently the main map in TxOrphanage is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we'll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid.

This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test.

Prior to #28970, we don't rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node's orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we'll end up accepting it. However, this is a bit more painful for 1p1c: it's easier for an attacker to tell when a tx is going to hit a node's orphanage, and we need to store the correct orphan + receive the parent before we'll consider the package. If we start out with a bad orphan, we can't evict it until we receive the parent + try the 1p1c, and then we'll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c.

DrahtBot · 2024-04-29T20:59:12Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, AngusP, theStack, sr-gi, stickies-v, itornaza
Stale ACK	mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot · 2024-04-29T23:14:30Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24396449433}

glozow · 2024-04-30T15:26:32Z

Will fix the linter when I rebase

instagibbs

code changes are quite mechanical, thankfully

concept ACK

instagibbs · 2024-05-01T16:19:56Z

src/net_processing.cpp

- if (m_orphanage.HaveTx(gtxid)) return true;
+ // Orphanage is checked by wtxid. However, even if this is a txid, look up the same hash in
+ // case this is a non-segwit transaction in the orphanage.
+ if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;


Suggested change

if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;

if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;

instagibbs · 2024-05-01T18:20:39Z

should be able to add fuzz coverage easily for differing wtxids but same txids in src/test/fuzz/txorphan.cpp

stickies-v

Concept ACK, but I'm concerned about the behaviour change in AlreadyHaveTx, which seems quite non-trivial to review so I'll need to look into further.

src/txorphanage.h

src/txorphanage.cpp

stickies-v · 2024-05-01T15:21:51Z

src/txorphanage.cpp

 if (maybeErase->second.fromPeer == peer)
 {
- nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
+ nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());


nit: can't we just use first?

Suggested change

nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());

nErased += EraseTxNoLock(maybeErase->first);

Or alternatively:

git diff on 4b4dfaa

diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 504a1f7ec3..bed92e6b32 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -98,13 +98,11 @@ void TxOrphanage::EraseForPeer(NodeId peer) m_peer_work_set.erase(peer); int nErased = 0; - std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin(); - while (iter != m_orphans.end()) - { - std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid - if (maybeErase->second.fromPeer == peer) + for (auto iter = m_orphans.begin(); iter != m_orphans.end(); ) { + const auto& [wtxid, tx] = *iter++; // increment to avoid iterator becoming invalid + if (tx.fromPeer == peer) { - nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash()); + nErased += EraseTxNoLock(wtxid); } } if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);

this could still happen

this was (mostly) taken

src/test/fuzz/txorphan.cpp

src/net_processing.cpp

itornaza · 2024-05-03T18:14:57Z

concept ACK

After being caught out of guard due to the short notice on this PR at the Bitcoin Review Club, I took time to review the code changes and read the comments above.

To my understanding, we need to check the wtxid for screening transactions for inclusion in the orphanage in order to catch witness malleations as well. This was not previously possible because we where only checking against txid that does not include the [witness] section of a transaction as defined in BIP141.

This change needs to be implemented only for the orphanage and not the rest of the structures that AlreadyHaveTx() checks against, since txid might be relevant there. The new behavior is implicitly documented in the bool TxOrphanage::HaveTx(const Wtxid& wtxid) const function definition which now takes wtxid as its single argument.

sr-gi

Concept ACK

src/txorphanage.cpp

sr-gi · 2024-05-03T17:54:15Z

src/txorphanage.cpp

 if (maybeErase->second.fromPeer == peer)
 {
- nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
+ nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
 }
 }
 if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);


In 646aa4d

nit: nErased is a counter right? Shouldn't this read txs instead?

Ok I've added a commit to fix this and another log to say "transaction(s)". I took the opportunity to add a few nice things to the logs as well.

src/txorphanage.cpp

glozow

Ok I have split the first commit into:

net processing change to stop querying by txid
2 refactors changing the TxOrphanage methods to be by Wtxid only
TxOrphanage change to index by wtxid and thus allow entries with the same txid

Hopefully in review, we can convince ourselves that the first commit is the right thing to do, separately from the other changes. The tests may help with that - also added one for invs that helps illustrate what the cases are, and added unit tests.

I thought a bit about whether we should check for same-txid-different-witness but from the same peer and replace that in orphanage. Given that a Bitcoin Core node doesn't ever do witness replacement, this seems like unnecessary complexity that won't ever get used between normally-operating nodes. So I did not add this.

glozow · 2024-05-10T11:15:01Z

src/net_processing.cpp

- if (m_orphanage.HaveTx(gtxid)) return true;
+ // Orphanage is checked by wtxid. However, even if this is a txid, look up the same hash in
+ // case this is a non-segwit transaction in the orphanage.
+ if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;


src/test/fuzz/txorphan.cpp

src/txorphanage.cpp

src/txorphanage.h

instagibbs

reviewed through 65f69bf

Given that a Bitcoin Core node doesn't ever do witness replacement, this seems like unnecessary complexity that won't ever get used between normally-operating nodes. So I did not add this.

Makes sense, seems unlikely to ever be used in a 20 minute window lifetime of an orphan given that we don't do these kinds of replacements.

also opened #30082 seeing we're touching EraseForPeer et al a bit in not quite "trivial" ways

instagibbs · 2024-05-10T14:47:16Z

src/txorphanage.cpp

 if (maybeErase->second.fromPeer == peer)
 {
- nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
+ nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());


this could still happen

test/functional/p2p_orphan_handling.py

theStack

Concept ACK

Code changes look good at first glance, still thinking about the implications of 9d2e905 (w.r.t. discussion in #30000 (comment)). Verified that both the unit tests and each of the newly introduced functional sub-tests fail without the main commit 4206ba3, as expected.

test/functional/p2p_orphan_handling.py

glozow · 2024-05-13T14:00:26Z

Thanks @instagibbs @theStack @sr-gi! Addressed the comments and added a few minor log improvements.

glozow · 2024-05-13T17:03:10Z

Ah, p2p_invalid_tx.py failed because assert_debug_log didn't match anymore (those should be unit tests!)

instagibbs

reviewed through 16483fe

git range-diff master 65f69bf 16483fee7c6d93722bfb79fce9efbe841ec13d6a

test/functional/p2p_orphan_handling.py

src/txorphanage.cpp

AngusP

tACK 16483fe with some nits

src/net_processing.cpp

test/functional/p2p_orphan_handling.py

mzumsande

Tested ACK 16483fe

I was at first skeptical about the first commit for similar reasons as stickies-v above, but after thinking about it more and reading the discussion it makes sense to me to cast a Txid to a Wtxid here.
I also played with the functional tests a bit (e.g. giving the parent in test_orphan_txid_inv a low fee and testing that everything still works if 1p1c package validation is used).

test/functional/p2p_orphan_handling.py

No behavior change right now, as transactions in the orphanage are unique by txid. This makes the next commit easier to review.

Index by wtxid instead of txid to allow entries with the same txid but different witnesses in orphanage. This prevents an attacker from blocking a transaction from entering the orphanage by sending a mutated version of it.

glozow

Thanks @mzumsande @AngusP @instagibbs! Addressed comments. I've also expanded the "cast from txid to wtxid" part to be slightly more verbose but hopefully easier to understand.

test/functional/p2p_orphan_handling.py

src/txorphanage.cpp

src/net_processing.cpp

- Add elapsed time in "remove orphan" log - Add size in "stored orphan" log - grammar edit

instagibbs

ACK 0fb17bf

reviewed via git range-diff master 16483fee7c6d93722bfb79fce9efbe841ec13d6a 0fb17bf61a40b73a2b81a18e70b3de180c917f22

glozow · 2024-05-14T15:20:20Z

src/net_processing.cpp

+ if (gtxid.IsWtxid()) {
+ // Normal query by wtxid.
+ if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
+ } else {
+ // Never query by txid: it is possible that the transaction in the orphanage has the same
+ // txid but a different witness, which would give us a false positive result. If we decided
+ // not to request the transaction based on this result, an attacker could prevent us from
+ // downloading a transaction by intentionally creating a malleated version of it. While
+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+ // discerning which one that is, so the orphanage can store multiple transactions with the
+ // same txid.
+ //
+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
+ if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
+ }


(since @sr-gi mentioned this offline) Yes, the last push turned this 1 line into 2 identical lines. The purpose is to illustrate to current reviewers and future readers more clearly... if wtxid, we do this. if txid, we also do this, but notice that we are actually doing a "cast" and that's ok because we are guessing what the wtxid is.

Also, perhaps these will diverge again in the future if GenTxid becomes a std::variant<Txid, Wtxid>.

AngusP · 2024-05-14T16:20:10Z

ACK 0fb17bf

theStack

ACK 0fb17bf

I like the latest, more verbose version of the first commit (7e475b9), as it was significantly easier to grasp what's going on and convince myself that this is the behaviour we want. The only small drawback I see that it looks odd if both the if- and else-branch do exactly the same thing, and it could invite people to open deduplication refactor PRs (or leave them wondering if it's a bug). Maybe adding some "this is intended" comment would make sense for a follow-up.

theStack · 2024-05-14T16:42:15Z

test/functional/p2p_orphan_handling.py

+ tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"])
+
+ # Create a fake version of the child
+ tx_orphan_bad_wit = self.create_malleated_version(tx_child)


nit, potential follow-up material: the test is imho a bit easier to follow if the naming refers to the original (non-malleated) counter-part:

Suggested change

tx_orphan_bad_wit = self.create_malleated_version(tx_child)

tx_child_bad_wit = self.create_malleated_version(tx_child)

(same as for the other two sub-tests below)

AngusP · 2024-05-14T17:48:37Z

Maybe adding some "this is intended" comment would make sense for a follow-up.

Could make the 'cast' explicit in the second branch, though there's already the comment making it pretty clear this is intentional so may be unnecessary

const auto guessed_wtxid = Wtxid::FromUint256(hash);
if (m_orphanage.HaveTx(guessed_wtxid)) return true;

sr-gi

crACK 0fb17bf

Non-blocking comments

test/functional/p2p_orphan_handling.py

stickies-v

ACK 0fb17bf

It took me a while to grok the implications of this PR, but the latest version of this PR makes it much more reviewable and I'm now comfortable that the changes are safe and desired. Also, nice work on the tests!

What I missed/misunderstood in my earlier review is that the reduced orphanage hit rate is a goal, and not a bug/something to minimize. Related nit: perhaps the commit message of 7e475b9 could be explicit about this reduction and why we want it?

I remain uneasy about AlreadyHaveTx() not really doing what the name indicates (because of the GenTxid ambiguity), but is not a huge concern and I think should not block this PR / can be addressed separately if it is desired.

src/txorphanage.cpp

stickies-v · 2024-05-13T16:43:40Z

src/test/orphanage_tests.cpp

@@ -180,6 +190,49 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
 BOOST_CHECK(orphanage.CountOrphans() == 0);
 }

+BOOST_AUTO_TEST_CASE(same_txid_diff_witness)
+{
+ FastRandomContext det_rand{true};


tidy nit:

Suggested change

FastRandomContext det_rand{true};

FastRandomContext det_rand{/*fDeterministic=*/true};

stickies-v · 2024-05-14T14:36:23Z

src/net_processing.cpp

+ if (gtxid.IsWtxid()) {
+ // Normal query by wtxid.
+ if (m_orphanage.HaveTx(gtxid)) return true;
+ } else {
+ // Never query by txid: it is possible that the transaction in the orphanage has the same
+ // txid but a different witness, which would give us a false positive result. If we decided
+ // not to request the transaction based on this result, an attacker could prevent us from
+ // downloading a transaction by intentionally creating a malleated version of it.
+ //
+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
+ if (m_orphanage.HaveTx(GenTxid::Wtxid(hash))) return true;
+ }


In 7e475b9:

I think having the if/else path in this commit is helpful, since it distinguishes between when we cast the txid and when we don't.

nit: I think this already works pretty well, but some bits (e.g. "guess what the wtxid is") are not super clear imo. I've summarized my understanding into the below, feel free to use what you like.

// Never query by txid: it is possible that an existing transaction in the orphanage has the // same txid but a different witness, which would give us a false positive result. If we // don't request the transaction based on this result, an attacker could prevent us from // downloading a transaction by intentionally creating a malleated version of it. // // False positive are unsafe and must be avoided.False negatives are acceptable and just // lead to increased bandwidth usage. // // When gtxid is a txid it means when we don't have enough information to query by wtxid, // e.g. because the transaction was announced by a non-wtxid-relay peer, or because we're // checking the prevouts of a transaction. It doesn't mean that the transaction has no // witness data, so we need to distinguish between those cases. // // If the transaction: // 1) does not have witness data, we can safely query the orphanage by casting a txid to a // wtxid because they are equivalent. False positives and false negatives are impossible. // 2) has witness data, the txid and wtxid are different by definition, making this query a // no-op. False positives are not possible, but we may have false negatives. // // So, by querying for a txid cast to a wtxid, we avoid false positives entirely, but we // save bandwidth for the case where txid == wtxid.

side-note rant: I think the fact that we need such a verbose comment is a pretty strong sign AlreadyHaveTx should be refactored, e.g. using {Wt,T}xid types as I suggested here, but I appreciate that it's mostly orthogonal to this PR. Renaming AlreadyHaveTx to e.g. ShouldRequestTx or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

I've summarized my understanding into the below

Looks correct to me

I think the fact that we need such a verbose comment is a pretty strong sign AlreadyHaveTx should be refactored, e.g. using {Wt,T}xid types as I suggested #30000 (comment), but I appreciate that it's mostly orthogonal to this PR.

I think it's really a sign to ban txid-based transaction relay. Just kidding. I agree it's hard to reason about logic dealing with a hash that can be txid, wtxid, or bot. I think it's just one of the common things that trips people in tx relay, just like everybody's mempool fuzzer gets snagged on CPFP carve out, and that for loop in init.cpp that claims reviewer victims every time it's touched.

Renaming AlreadyHaveTx to e.g. ShouldRequestTx or something would probably already clarify quite a bit, too (although also not blocking for this PR either).

Yeah, AlreadyHaveTx should probably have a name that reflects its purpose of "should I bother {requesting, downloading, validating} this tx".

Do note that #30110 makes AlreadyHaveTx a function internal to the TxDownloadImpl, i.e. no longer something that peerman knows about. Funnily, TxDownloadImpl::AlreadyHaveTx might be a fine name when it's in that context. I get that it's confusing, but I'd ask that we defer renaming until after that PR since it would conflict + maybe be less problematic afterwards.

stickies-v · 2024-05-14T15:21:56Z

src/txorphanage.cpp

@@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)

 const Txid& hash = tx->GetHash();
 const Wtxid& wtxid = tx->GetWitnessHash();
- if (m_orphans.count(hash))
+ if (m_orphans.count(wtxid))


c++-20 nit:

Suggested change

if (m_orphans.count(wtxid))

if (m_orphans.contains(wtxid))

stickies-v · 2024-05-14T15:22:24Z

src/txorphanage.cpp

- } else {
- return m_orphans.count(Txid::FromUint256(gtxid.GetHash()));
- }
+ return m_orphans.count(wtxid);


c++20-nit:

Suggested change

return m_orphans.count(wtxid);

return m_orphans.contains(wtxid);

stickies-v · 2024-05-14T15:32:50Z

src/net_processing.cpp

+ // downloading a transaction by intentionally creating a malleated version of it. While
+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+ // discerning which one that is, so the orphanage can store multiple transactions with the
+ // same txid.


In 8923edf:

nit: I feel like documentation on why TxOrphanage indexes by wtxid/allows multiple versions of the same transaction should be documented in TxOrphanage instead of here.

stickies-v · 2024-05-14T16:31:35Z

test/functional/p2p_orphan_handling.py

+ bad_peer = node.add_p2p_connection(P2PInterface())
+ honest_peer = node.add_p2p_connection(P2PInterface())
+
+ # 1. Fake orphan is received first. It is missing an input.


nit: I first interpreted that as that an input had been removed from the transaction, perhaps better to use "parent" as to stay consistent with the language in the next steps?

Suggested change

# 1. Fake orphan is received first. It is missing an input.

# 1. Fake orphan is received first, the parent is not yet broadcast.

stickies-v · 2024-05-14T16:45:48Z

test/functional/p2p_orphan_handling.py

+ tx_middle = self.wallet.create_self_transfer(utxo_to_spend=tx_grandparent["new_utxo"])
+
+ # Create a fake version of the middle tx
+ tx_orphan_bad_wit = self.create_malleated_version(tx_middle)


nit: would tx_middle_bad_wit be more consistent naming here?

itornaza · 2024-05-14T18:34:42Z

trACK 0fb17bf

ryanofsky · 2024-05-15T13:58:55Z

I merged this as-is since it has so many acks, but it probably makes sense to follow up on the last few comments and suggestions.

DrahtBot added the P2P label Apr 29, 2024

osarukun approved these changes Apr 29, 2024

View reviewed changes

DrahtBot added the CI failed label Apr 29, 2024

DrahtBot mentioned this pull request Apr 30, 2024

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

glozow mentioned this pull request Apr 30, 2024

txorphanage: add size accounting, use wtxids, support multiple announcers #28481

Closed

glozow force-pushed the 2024-04-orphan-use-wtxid branch from c1bd985 to 4b4dfaa Compare May 1, 2024 10:15

DrahtBot removed the CI failed label May 1, 2024

glozow marked this pull request as ready for review May 1, 2024 12:49

glozow mentioned this pull request May 1, 2024

Package Relay Project Tracking #27463

Open

53 tasks

instagibbs reviewed May 1, 2024

View reviewed changes

stickies-v reviewed May 1, 2024

View reviewed changes

sr-gi reviewed May 3, 2024

View reviewed changes

glozow force-pushed the 2024-04-orphan-use-wtxid branch from 4b4dfaa to 65f69bf Compare May 10, 2024 11:40

glozow commented May 10, 2024

View reviewed changes

instagibbs mentioned this pull request May 10, 2024

test: expand LimitOrphan and EraseForPeer coverage #30082

Open

instagibbs reviewed May 10, 2024

View reviewed changes

theStack reviewed May 12, 2024

View reviewed changes

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

glozow force-pushed the 2024-04-orphan-use-wtxid branch from 65f69bf to 822b2f1 Compare May 13, 2024 13:57

DrahtBot added the CI failed label May 13, 2024

glozow force-pushed the 2024-04-orphan-use-wtxid branch from 822b2f1 to 16483fe Compare May 13, 2024 17:02

instagibbs reviewed May 13, 2024

View reviewed changes

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

src/txorphanage.cpp Outdated Show resolved Hide resolved

AngusP approved these changes May 13, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

DrahtBot requested a review from theStack May 13, 2024 22:04

mzumsande reviewed May 13, 2024

View reviewed changes

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

test/functional/p2p_orphan_handling.py Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label May 14, 2024

glozow added 5 commits May 14, 2024 10:31

[p2p] don't query orphanage by txid

7e475b9

[refactor] TxOrphanage::HaveTx only by wtxid

efcc593

[refactor] TxOrphanage::EraseTx by wtxid

c31f148

No behavior change right now, as transactions in the orphanage are unique by txid. This makes the next commit easier to review.

[p2p] allow entries with the same txid in TxOrphanage

8923edf

Index by wtxid instead of txid to allow entries with the same txid but different witnesses in orphanage. This prevents an attacker from blocking a transaction from entering the orphanage by sending a mutated version of it.

[unit test] TxOrphanage handling of same-txid-different-witness txns

6675f64

glozow force-pushed the 2024-04-orphan-use-wtxid branch from 16483fe to 51c73a0 Compare May 14, 2024 09:34

glozow commented May 14, 2024

View reviewed changes

glozow added 2 commits May 14, 2024 10:38

[functional test] attackers sending mutated orphans

b16da7e

[log] updates in TxOrphanage

0fb17bf

- Add elapsed time in "remove orphan" log - Add size in "stored orphan" log - grammar edit

glozow force-pushed the 2024-04-orphan-use-wtxid branch from 51c73a0 to 0fb17bf Compare May 14, 2024 09:39

DrahtBot added the CI failed label May 14, 2024

This comment was marked as outdated.

Sign in to view

DrahtBot removed the CI failed label May 14, 2024

instagibbs approved these changes May 14, 2024

View reviewed changes

DrahtBot requested review from AngusP and mzumsande May 14, 2024 11:57

glozow commented May 14, 2024

View reviewed changes

theStack approved these changes May 14, 2024

View reviewed changes

sr-gi reviewed May 14, 2024

View reviewed changes

test/functional/p2p_orphan_handling.py Show resolved Hide resolved

test/functional/p2p_orphan_handling.py Show resolved Hide resolved

stickies-v approved these changes May 14, 2024

View reviewed changes

glozow mentioned this pull request May 15, 2024

refactor: TxDownloadManager #30110

Draft

ryanofsky self-assigned this May 15, 2024

ryanofsky merged commit 33303b2 into bitcoin:master May 15, 2024
16 checks passed

glozow deleted the 2024-04-orphan-use-wtxid branch May 15, 2024 15:43

	if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;
	if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;

	nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
	nErased += EraseTxNoLock(maybeErase->first);

	tx_orphan_bad_wit = self.create_malleated_version(tx_child)
	tx_child_bad_wit = self.create_malleated_version(tx_child)

	FastRandomContext det_rand{true};
	FastRandomContext det_rand{/fDeterministic=/true};

	return m_orphans.count(wtxid);
	return m_orphans.contains(wtxid);

	# 1. Fake orphan is received first. It is missing an input.
	# 1. Fake orphan is received first, the parent is not yet broadcast.

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

Conversation

glozow commented Apr 29, 2024 • edited

DrahtBot commented Apr 29, 2024 • edited

Code Coverage

Reviews

Conflicts

DrahtBot commented Apr 29, 2024

glozow commented Apr 30, 2024

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented May 1, 2024

stickies-v left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itornaza commented May 3, 2024

sr-gi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

glozow commented May 13, 2024

glozow commented May 13, 2024

instagibbs left a comment • edited

Choose a reason for hiding this comment

AngusP left a comment

Choose a reason for hiding this comment

mzumsande left a comment • edited

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngusP commented May 14, 2024

theStack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngusP commented May 14, 2024

sr-gi left a comment

Choose a reason for hiding this comment

stickies-v left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itornaza commented May 14, 2024

ryanofsky commented May 15, 2024

glozow commented Apr 29, 2024 •

edited

DrahtBot commented Apr 29, 2024 •

edited

stickies-v left a comment •

edited

instagibbs left a comment •

edited

mzumsande left a comment •

edited

stickies-v left a comment •

edited