Use wtxid for transaction relay #18044

sdaftuar · 2020-01-31T16:42:02Z

Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.

We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.

As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).

Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.

Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.

To do items:

Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.

DrahtBot · 2020-01-31T17:55:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove mempool global #19556 (Remove mempool global by MarcoFalke)
Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
Refactor mempool.dat to be extensible, and store missing info #19488 (Refactor mempool.dat to be extensible, and store missing info by luke-jr)
Remove CTxMempool::mapLinks data structure member #19478 (Remove CTxMempool::mapLinks data structure member by JeremyRubin)
refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
Overhaul transaction request logic #19184 (Overhaul transaction request logic by sipa)
test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134 (test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until by dboures)
Implement ADDRv2 support (part of BIP155) #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
Accurately account for mempool index memory #18086 (Accurately account for mempool index memory by sipa)
RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/net_processing.cpp

sdaftuar · 2020-02-04T14:23:39Z

Wrote up a short BIP draft here: https://github.com/sdaftuar/bips/blob/2020-02-wtxid-relay/bip-wtxid-relay.mediawiki, which this PR implements.

I'll wait for some concept ACKs here before I send to the mailing list.

jonatack · 2020-02-04T14:38:06Z

Nice -- will add a link to the BIP draft in the review club notes at https://bitcoincore.reviews/18044. Currently reviewing. Can someone add a Review club label? It helps let people know that there is a review club resource they can use.

rajarshimaitra · 2020-02-04T18:27:39Z

Cocnept ACK. Ran standard tests. All passing. Overall agreed with the idea of using wtxid for everything. Will try to try out the huristic and report some results.

rajarshimaitra · 2020-02-04T18:29:58Z

src/net_processing.cpp

                // Do not use rejection cache for witness transactions or
                // witness-stripped transactions, as they can have been malleated.
                // See https://github.com/bitcoin/bitcoin/issues/8279 for details.


Doesn't this comment needs update in light of the changed condition? We are now accepting txs containg witness in recentRejects.

adamjonas · 2020-02-06T19:47:38Z

Reference IRC discussion for further context around the VERSION/VERACK messages.

ajtowns · 2020-02-09T19:36:59Z

Concept ACK. First take: doing extra messages in between VERSION and VERACK sounds better to me than extending VERSION or potentially doing txid relay briefly in between VERACK and WTXIDRELAY, the code changes seem really nicely laid out, and the main thing that caught my eye looks like it's probably fixed by the WITNESS_STRIPPED addition.

TheBlueMatt

Didn't-get-a-chance-to-go-through-the-whole-thing-and-am-too-tired-to-be-reviewing-anyway Concept ACK

(that's how we're supposed to ACK now, right, with lots of context?)

src/txmempool.h

src/net.h

TheBlueMatt · 2020-02-12T23:34:54Z

src/protocol.h

@@ -363,7 +369,8 @@ enum GetDataMsg
    UNDEFINED = 0,
    MSG_TX = 1,
    MSG_BLOCK = 2,
-    // The following can only occur in getdata. Invs always use TX or BLOCK.
+    MSG_WTX = 5, // Defined in BIP XXX


WITNESS_TX and WTX read a little bit too close for comfort. I don't think the extra characters to write a useful name cost us much :p.

Suggestions?

Would be okay with MSG_WTXID to mark we only deal with tx identifier not a specific new data structure, we already have a GetDataMsg for the transaction and its witness.

I think MSG_WTXID is even more confusing than MSG_WTX. The suffix here represents an object being fetched. So TX or BLOCK with some type. MSG_WTXID would mean we are fetching an ID.
The only thing I can thing of is MSG_TX_BY_WTXID. But I'm also fine with MSG_WTX.

Maybe I've been staring at this for too long but I agree with @naumenkogs. I confused MSG_TX and MSG_WTX once during the first pass at reviewing a few weeks ago, but then I got used to it and I like its brevity. I think it's fine for now.

TX (by txid, don't send witness), WTX_TXID (by txid, do send witness), WTX_WTXID (by wtxid, do send witness)` might be clearer?

TheBlueMatt · 2020-02-12T23:58:43Z

src/net_processing.cpp

@@ -1184,6 +1204,7 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pb
        LOCK(g_cs_recent_confirmed_transactions);
        for (const auto ptx : pblock->vtx) {


Not (this pr's) your bug, but clang complains that you're copying shared_ptrs on every iteration and you should add a &.

(this is already fixed on master, so a rebase would solve it)

src/net_processing.cpp

TheBlueMatt · 2020-02-13T00:17:42Z

src/net_processing.cpp

+                // do not support wtxid-based relay. See
+                // https://github.com/bitcoin/bitcoin/issues/8279 for details.
+                // We can remove this restriction (and always add wtxids to
+                // the filter even for witness tripped transactions) once


s/tripped/stripped/, but, more importantly, can we? Wouldn't that allow an attacker to see an 0.19 client's inv, request the tx without delay (unlike its other peers), then spam the network with the witness-stripped version, preventing relay of that transaction. At least as long as we dont ban for TX_WITNESS_STRIPPED.

From a single node's perspective, as long as you have at least 1 honest wtxid-relay peer, such an attack would not work, because the wtxid for the honest version of the transaction would not be added to your filter (only the txid), and so you would eventually request the valid version of the transaction from your honest wtxid-relay peer.

Currently we preference outbound peers for transaction download, so once we believe that some sufficient majority of listening nodes support wtxid-relay, such that the chances of having 0 or just a few wtxid-relay-outbound-peers is pretty small, then I think we can just get rid of this.

I don't think our threat model has ever really been too concerned with every node getting every transaction, anyway -- we just don't want the network as a whole to not relay a transaction altogether because of something like this. So exactly how long we should wait before dropping this logic is debatable IMO; anyway we should defer the debate to the future when someone thinks this is ready.

TheBlueMatt · 2020-02-13T00:28:23Z

FWIW, travis error is " node0 2020-02-12T14:39:48.835709Z [msghand] Error: Disk space is too low! "

fjahr · 2020-02-14T16:58:04Z

Concept ACK on the BIP draft and the implementation of it

I am not so sure about the 2-second delay, however, because it is not necessarily an improvement, it can also make matters worse afaict. Example: At least one of our txid peers is better connected than all of our wtxid peers. We get a txid inv first and wait for 2 seconds. Meanwhile, we receive the wtxid inv and request it, but before we get the response we also request the transaction from the txid peer. I am not sure how realistic this is and it will certainly not happen frequently once large parts of the network have upgraded to wtxid invs. But I also think the PR is compelling enough without the 2-second delay optimization and it could be left as a follow-up so it can be discussed separately.

Do you plan to add additional tests? Otherwise, I might be interested to look into that.

instagibbs · 2020-02-18T20:03:46Z

concept ACK

naumenkogs · 2020-02-25T19:46:27Z

Concept ACK

ariard · 2020-07-23T01:04:01Z

Posthumous Code Review ACK 0a4f142

Another follow-up:

update tracking of multiple transactions with same txid, especially BroadcastTransaction, see Use wtxid for transaction relay #18044 (comment)

0a4f142 Further improve comments around recentRejects (Suhas Daftuar) 0e20cfe Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar) cacd852 test: Use wtxid relay generally in functional tests (Fabian Jahr) 8d8099e test: Add tests for wtxid tx relay in segwit test (Fabian Jahr) 9a5392f test: Update test framework p2p protocol version to 70016 (Fabian Jahr) dd78d1d Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar) 4eb5155 Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar) 97141ca Delay getdata requests from peers using txid-based relay (Suhas Daftuar) 46d78d4 Add p2p message "wtxidrelay" (Suhas Daftuar) 2d282e0 ignore non-wtxidrelay compliant invs (Anthony Towns) ac88e2e Add support for tx-relay via wtxid (Suhas Daftuar) 8e68fc2 Add wtxids to recentRejects instead of txids (Suhas Daftuar) 144c385 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar) 85c78d5 Add wtxid-index to orphan map (Suhas Daftuar) 08b3995 Add a wtxid-index to mapRelay (Suhas Daftuar) 60f0acd Just pass a hash to AddInventoryKnown (Suhas Daftuar) c7eb6b4 Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar) 2b4b90a Add a wtxid-index to the mempool (Suhas Daftuar) Pull request description: Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool. We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in bitcoin#8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this. As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course). Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade. Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome. To do items: - [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type) - [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes. ACKs for top commit: naumenkogs: utACK 0a4f142 laanwj: utACK 0a4f142 Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df

jnewbery · 2020-07-26T08:07:36Z

src/txmempool.h

+     * track locally submitted transactions to periodically retry initial broadcast
+     * map of txid -> wtxid
+     */
+    std::map<uint256, uint256> m_unbroadcast_txids GUARDED_BY(cs);


I don't think this change is necessary (or desirable). I guess it was prompted by this comment: #18038 (comment) , but storing this as a map from txid->wtxid is unnecessary and confusing, since it obfuscates the purpose of this data, which is a set of transactions that may need to be rebroadcast.

The only place that the wtxid is read from this map is in ReattemptInitialBroadcast(), at which point we can fetch the transaction from the mempool (we very nearly do that with m_mempool.exists()), and get the wtxid from there.

cc @amitiuttarwar @ariard

My mistake here, effectively you need to announce transactions from the unbroadcast set by wtxid to upgraded peers but as you point out it doesn't mean we need to cache them as a solution.

Maybe we should precise unbroadcast set semantic wrt same txid, different wtxids, we only guarantee reattempt to the best mempool candidate known for a given txid at the time we call ReattemptInitialBroadcast, not insertion (AddUnbroadcastTx). #18044 (comment) isn't an issue.

I really think this "same txid, different wtxid" thing is a complete red herring. The mempool can only ever be aware of one witness for a transaction, so any attempt to announce the transaction via a different wtxid would fail anyway. As Suhas pointed out earlier (#18044 (comment)), to support tracking multiple witnesses we'd need to make significant changes many of our components. Besides, the unbroadcast set is transactions that we created ourselves, so we're not expecting the witnesses to change.

Agree with @jnewbery. There is no real way that we can reasonable start tracking multiple witnesses for the same transaction (either in the mempool or the wallet, ...), so unless this map's storing of wtxid is needed for efficiency, it seems just a set of txid should be sufficient here.

😮 wow that's a great point @jnewbery. thank you!!

I thought during implementation I ran into something that prevented me from exclusively having txids on the set, but that evaluation is simply wrong. I've taken a first pass at reverting unbroadcast to a set here: amitiuttarwar@f51cccd. I'll need to revisit with fresh eyes and then will PR.

I agree set makes more sense than a map- its simpler, communicates the intent better, and there's no noticeable efficiency gain with the map. Since we check the transaction is the mempool, the difference between map vs set is the difference between calling CTxMemPool::get vs CTxMemPool::exists. This boils down to mapTx.find(hash) vs mapTx.count(hash), which both have log(n) complexity (according to the boost docs I found).

@amitiuttarwar mapTX is a multiindex and we use a hashed index not a ordered index so it's O(1) lookup. Not sure exactly how mapTX relates to m_unboardcast_txids though...

@amitiuttarwar that patch looks correct from a quick skim. I'll review fully once you open a PR.

As @JeremyRubin points out, we're using a boost::multi_index::hashed_unique index for the wtxid index, which has constant time search/retrieval.

reverted to set in cb79b9d, #19879

jnewbery · 2020-07-26T08:15:35Z

src/net_processing.cpp

    {
-        pnode->PushTxInventory(txid);
+        AssertLockHeld(cs_main);
+        CNodeState &state = *State(pnode->GetId());


We shouldn't be indiscriminately taking the address of a return value which may be nullptr. I know the same pattern exists in a few other places, but really we should check for existence here before dereferencing the pointer.

addressed in fc66d0a, #19879

…use in net processing c251d71 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack) 4254cd9 p2p: add CInv transaction message helper methods (Jon Atack) Pull request description: Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation: - add `CInv` transaction message helper methods, defined in the class - use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`. ACKs for top commit: fjahr: Code review ACK c251d71 laanwj: Code review ACK c251d71 vasild: ACK c251d71 theStack: Code-Review ACK c251d71 hebasto: ACK c251d71, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d

…lpers; use in net processing c251d71 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack) 4254cd9 p2p: add CInv transaction message helper methods (Jon Atack) Pull request description: Following the merge of wtxid relay in bitcoin#18044, this is the first of three refactoring PRs (this one, bitcoin#19610, and bitcoin#19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation: - add `CInv` transaction message helper methods, defined in the class - use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`. ACKs for top commit: fjahr: Code review ACK c251d71 laanwj: Code review ACK c251d71 vasild: ACK c251d71 theStack: Code-Review ACK c251d71 hebasto: ACK c251d71, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d

jnewbery · 2020-07-31T10:27:24Z

src/net_processing.cpp

-                // Do not use rejection cache for witness transactions or
-                // witness-stripped transactions, as they can have been malleated.
-                // See https://github.com/bitcoin/bitcoin/issues/8279 for details.
+            if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {


The else if (tx.HasWitness() && RecursiveDynamicUsage(*ptx) < 100000) block below is now dead code and should be removed (anything that doesn't go in this if branch is TX_WITNESS_STRIPPED and therefore doesn't have a witness)

addressed in: 125c038, #19879

10b7a6d refactor: make txmempool interface use GenTxid (Pieter Wuille) 5c124e1 refactor: make FindTxForGetData use GenTxid (Pieter Wuille) a2bfac8 refactor: use GenTxid in tx request functions (Pieter Wuille) e65d115 test: request parents of orphan from wtxid relay peer (Anthony Towns) 900d7f6 p2p: enable fetching of orphans from wtxid peers (Pieter Wuille) 9efd86a refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic (Pieter Wuille) d362f19 doc: list support for BIP 339 in doc/bips.md (Pieter Wuille) Pull request description: This is based on #18044 (comment). A new type `GenTxid` is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted. Also document BIP339 in doc/bips.md. ACKs for top commit: jnewbery: Code review ACK 10b7a6d jonatack: ACK 10b7a6d ajtowns: ACK 10b7a6d -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice. naumenkogs: utACK 10b7a6d Tree-SHA512: d518d13ffd71f8d2b3c175dc905362a7259689e6022a97a0b4f14f1f9fdd87475cf5af70cb12338d1e5d31b52c12e4faaea436114056a2ae9669cb506240758b

10b7a6d refactor: make txmempool interface use GenTxid (Pieter Wuille) 5c124e1 refactor: make FindTxForGetData use GenTxid (Pieter Wuille) a2bfac8 refactor: use GenTxid in tx request functions (Pieter Wuille) e65d115 test: request parents of orphan from wtxid relay peer (Anthony Towns) 900d7f6 p2p: enable fetching of orphans from wtxid peers (Pieter Wuille) 9efd86a refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic (Pieter Wuille) d362f19 doc: list support for BIP 339 in doc/bips.md (Pieter Wuille) Pull request description: This is based on bitcoin#18044 (comment). A new type `GenTxid` is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted. Also document BIP339 in doc/bips.md. ACKs for top commit: jnewbery: Code review ACK 10b7a6d jonatack: ACK 10b7a6d ajtowns: ACK 10b7a6d -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice. naumenkogs: utACK 10b7a6d Tree-SHA512: d518d13ffd71f8d2b3c175dc905362a7259689e6022a97a0b4f14f1f9fdd87475cf5af70cb12338d1e5d31b52c12e4faaea436114056a2ae9669cb506240758b

JeremyRubin

Post merge review up to ac88e2e

Couple questions

JeremyRubin · 2020-07-31T20:28:40Z

src/validation.cpp

          file >> unbroadcast_txids;
          unbroadcast = unbroadcast_txids.size();
-
-          for (const auto& txid : unbroadcast_txids) {


Clarification: does moving this out of the try block have any behavior change? Is it possible that a semi-corrupt unbroadcast_txids would partially deserialize and then have only some entries?

the try block was introduced to specifically catch the error when upgrading around not having an unbroadcast set written to disk. so I think having the initialization & iteration separate actually captures that intent better.

but lmk if you think of any specific ways this logic could be problematic

JeremyRubin · 2020-07-31T20:29:22Z

src/validation.cpp

-
-          for (const auto& txid : unbroadcast_txids) {
-            pool.AddUnbroadcastTx(txid);
-          }
        } catch (const std::exception&) {
          // mempool.dat files created prior to v0.21 will not have an


Suggest: clearing unbroadcast_txids here in case has garbage data.

looking into this- is there really a way that the CAutoFile >> operator could cause a silent/partial failure? as I mentioned previously the idea of this try catch is just for a smooth upgrade. To elaborate further: so we don't print Failed to deserialize mempool data on disk: %s. Continuing anyway." when everything actually went fine. the current intention is to remove this in 0.22, so if there's a possibility for unbroadcast_txids to have garbage data written, I'd like to think this through more carefully to find a long term solution.

Yes I believe so? Fortunately for std::map we deserialize an element and then insert it into the map, but is a possibility that we say that there should be 10 elements and there are only 5 elements, or there are 5 elements and their are actually 10. In the event the file is corrupt, we should likely treat it as if the entire thing is corrupt, rather than continuing to process.

Perhaps we should add an exception to throw from Unserialize if there are not the correct amount of entries?

I'm not sure to the degree we need to worry about files on our own local being corrupt, and there are other forms of corruption that can occur. But because it is a behavior change, I'm noting it.

to be clear the issue only comes up when you don't have enough elements or when you have a half element presently.

JeremyRubin · 2020-07-31T20:32:09Z

src/validation.cpp

-
+        for (const auto& elem : unbroadcast_txids) {
+            // Don't add unbroadcast transactions that didn't get back into the
+            // mempool.


Is this correct? Should this transaction get queued somewhere for rebroadcasting? Or is it a known precondition that because it failed to get into the mempool before this line it will fail again?

I can't think of why we'd want it in the unbroadcast if it didn't make it back in the mempool, but just double checking.

the unbroadcast set should always be a subset of the mempool. it just maintains txids, so we don't have the capability to do much if its not found.

Should this transaction get queued somewhere for rebroadcasting?

do you mean re-attempt mempool submission? bc we def wouldn't want to broadcast or rebroadcast a transaction we don't have in our mempool!

yes. it's possible that we may want to resubmit it to the mempool if it was previously in our mempool. What changed to cause it to no longer be accepted?

src/net_processing.cpp

JeremyRubin · 2020-07-31T20:51:38Z

src/net_processing.cpp

@@ -151,6 +151,7 @@ struct COrphanTx {
 };
 RecursiveMutex g_cs_orphans;
 std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);


Because presumably WTXID becomes the primary lookup method, someone may prefer to in the future switch the roles of mapOrphanTransactions and g_orphans_by_wtxid so that the g_orphans_by_wtxid is the owning map. This is a small performance win avoiding an extra iterator lookup.

test/functional/p2p_segwit.py

introduced in bitcoin#8525 then erroneously removed in PR bitcoin#18044. The restored line is how we are checking that the node will still re-request a specific txid given a witness-related failure.

a8a64ac [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar) 125c038 [p2p] Remove dead code (Amiti Uttarwar) fc66d0a [p2p] Check for nullptr before dereferencing pointer (Adam Jonas) cb79b9d [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar) Pull request description: Addresses some outstanding review comments from #18044 - reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map) - adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator) - removes some dead code Links to comments on wtxid PR: [1](#18044 (comment)) [2](#18044 (comment)) [3](#18044 (comment)) thanks to jnewbery & adamjonas for flagging these ! ! ACKs for top commit: sdaftuar: utACK a8a64ac naumenkogs: utACK a8a64ac jnewbery: utACK a8a64ac Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277

a8a64ac [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar) 125c038 [p2p] Remove dead code (Amiti Uttarwar) fc66d0a [p2p] Check for nullptr before dereferencing pointer (Adam Jonas) cb79b9d [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar) Pull request description: Addresses some outstanding review comments from bitcoin#18044 - reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map) - adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator) - removes some dead code Links to comments on wtxid PR: [1](bitcoin#18044 (comment)) [2](bitcoin#18044 (comment)) [3](bitcoin#18044 (comment)) thanks to jnewbery & adamjonas for flagging these ! ! ACKs for top commit: sdaftuar: utACK a8a64ac naumenkogs: utACK a8a64ac jnewbery: utACK a8a64ac Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277

fd9a006 Report and verify expirations (Pieter Wuille) 86f50ed Delete limitedmap as it is unused now (Pieter Wuille) cc16fff Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille) 173a1d2 Expedite removal of tx requests that are no longer needed (Pieter Wuille) de11b0a Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille) 242d164 Change transaction request logic to use txrequest (Pieter Wuille) 5b03121 Add txrequest fuzz tests (Pieter Wuille) 3c7fe0e Add txrequest unit tests (Pieter Wuille) da3b8fd Add txrequest module (Pieter Wuille) Pull request description: This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests). The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). * The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions). This replaces #19184, rebased on #18044 and with many small changes. ACKs for top commit: ariard: Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic. MarcoFalke: Approach ACK fd9a006 🏹 naumenkogs: Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events. jnewbery: utACK fd9a006 jonatack: WIP light ACK fd9a006 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers. ryanofsky: Light code review ACK fd9a006, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on: Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7

…cific MN p2p logic overhault fd9a006 Report and verify expirations (Pieter Wuille) 86f50ed Delete limitedmap as it is unused now (Pieter Wuille) cc16fff Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille) 173a1d2 Expedite removal of tx requests that are no longer needed (Pieter Wuille) de11b0a Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille) 242d164 Change transaction request logic to use txrequest (Pieter Wuille) 5b03121 Add txrequest fuzz tests (Pieter Wuille) 3c7fe0e Add txrequest unit tests (Pieter Wuille) da3b8fd Add txrequest module (Pieter Wuille) Pull request description: This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests). The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). * The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions). This replaces bitcoin#19184, rebased on bitcoin#18044 and with many small changes. ACKs for top commit: ariard: Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic. MarcoFalke: Approach ACK fd9a006 🏹 naumenkogs: Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events. jnewbery: utACK fd9a006 jonatack: WIP light ACK fd9a006 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers. ryanofsky: Light code review ACK fd9a006, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on: Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7

d4a1ee8 Further improve comments around recentRejects (Suhas Daftuar) f082a13 Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar) 22effa5 test: Use wtxid relay generally in functional tests (Fabian Jahr) e481681 test: Add tests for wtxid tx relay in segwit test (Fabian Jahr) 6be398b test: Update test framework p2p protocol version to 70016 (Fabian Jahr) e364b2a Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar) 879a3cf Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar) c1d6a10 Delay getdata requests from peers using txid-based relay (Suhas Daftuar) 181ffad Add p2p message "wtxidrelay" (Suhas Daftuar) 9382672 ignore non-wtxidrelay compliant invs (Anthony Towns) 2599277 Add support for tx-relay via wtxid (Suhas Daftuar) be1b7a8 Add wtxids to recentRejects (Suhas Daftuar) 7384521 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar) 606755b Add wtxid-index to orphan map (Suhas Daftuar) 3654937 Add a wtxid-index to mapRelay (Suhas Daftuar) f7833b5 Just pass a hash to AddInventoryKnown (Suhas Daftuar) 4df3d13 Add a wtxid-index to the mempool (Suhas Daftuar) Pull request description: We want wtxid relay to be widely deployed before taproot activation, so it should be backported to v0.20. The main difference from #18044 is removing the changes to the unbroadcast set (which was only added post-v0.20). The rest is mostly minor rebase conflicts (eg connman changed from a pointer to a reference in master, etc). We'll also want to backport #19569 after that's merged. ACKs for top commit: fjahr: re-ACK d4a1ee8 instagibbs: reACK d4a1ee8 laanwj: re-ACK d4a1ee8 hebasto: re-ACK d4a1ee8, only rebased since my [previous](#19606 (review)) review: Tree-SHA512: 1bb8725dd2313a9a03cacf8fb7317986eed3d8d1648fa627528441256c37c793bb0fae6c8c139d05ac45d0c7a86265792834e8e09cbf45286426ca6544c10cd5

Summary: ``` Since it's only used for transactions, there's no need to pass in an inv type. ``` Backport of core [[bitcoin/bitcoin#18044 | PR18044]]. Since thie PR is Segwit related, there is not much left. Full commits: - bitcoin/bitcoin@60f0acd - bitcoin/bitcoin@dd78d1d Only the CInv equality operator in messages.py is relevant: - bitcoin/bitcoin@9a5392f Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8501

Summary: This change prepares for upcoming commit "Do not hide compile-time thread safety warnings" by replacing AssertLockHeld() with LockAssertion() where needed. This is a backport of [[bitcoin/bitcoin#19668 | core#19668]] [1/5] bitcoin/bitcoin@af9ea55 Backport notse: - We do not have a lock in RelayTransaction because we did not backport [[bitcoin/bitcoin#18044 | core#18044]] (commit ac88e2eb619821ad7ae1d45d4b40be69051d3999) - This change is partially reverted in D10172 after D10171 makes it possible to annotate the lambda functions with `EXCLUSIVE_LOCKS_REQUIRED` Test Plan: With TSAN: `ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10161

DrahtBot added Mempool P2P labels Jan 31, 2020

sdaftuar force-pushed the 2020-01-wtxid-inv branch 2 times, most recently from 12080c0 to 310aafa Compare January 31, 2020 21:34

sipa reviewed Feb 1, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

sdaftuar commented Feb 1, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

sdaftuar force-pushed the 2020-01-wtxid-inv branch from b79ea9b to fd2c3f2 Compare February 3, 2020 20:51

jonatack reviewed Feb 4, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

rajarshimaitra reviewed Feb 4, 2020

View reviewed changes

sdaftuar force-pushed the 2020-01-wtxid-inv branch from fd2c3f2 to c4a23a1 Compare February 5, 2020 16:40

sipa mentioned this pull request Feb 7, 2020

Accurately account for mempool index memory #18086

Closed

sdaftuar force-pushed the 2020-01-wtxid-inv branch from 9e96b30 to cf8c70f Compare February 7, 2020 09:46

TheBlueMatt reviewed Feb 13, 2020

View reviewed changes

sdaftuar closed this Feb 18, 2020

sdaftuar reopened this Feb 18, 2020

jnewbery added the Review club label Feb 23, 2020

jnewbery reviewed Jul 26, 2020

View reviewed changes

jonatack mentioned this pull request Jul 26, 2020

p2p, refactor: add CInv transaction message helpers; use in net processing #19590

Merged

jnewbery mentioned this pull request Jul 28, 2020

Backport wtxid relay to v0.20 #19606

Merged

sdaftuar mentioned this pull request Jul 29, 2020

Add txids with non-standard inputs to reject filter #19620

Merged

jnewbery reviewed Jul 31, 2020

View reviewed changes

JeremyRubin reviewed Jul 31, 2020

View reviewed changes

instagibbs reviewed Aug 2, 2020

View reviewed changes

test/functional/p2p_segwit.py Show resolved Hide resolved

ariard mentioned this pull request Aug 2, 2020

Allow updating mempool-txn with cheaper witnesses #19645

Closed

ariard mentioned this pull request Aug 27, 2020

Transactions propagation design goals #19820

Closed

amitiuttarwar mentioned this pull request Sep 4, 2020

[p2p] miscellaneous wtxid followups #19879

Merged

sipa mentioned this pull request Sep 21, 2020

Overhaul transaction request logic #19988

Merged

jnewbery mentioned this pull request Nov 5, 2020

Backport wtxid orphan fetch to v0.20 #20317

Closed

This was referenced May 28, 2021

Switch to a (txid, auth_digest) identifier in places where the node needs to commit to a specific instance of a transaction zcash/zcash#5198

Open

[ZIP 239] Relay v5+ transactions with a (txid, auth_digest) tuple (or digest thereof) zcash/zcash#5199

Closed

str4d mentioned this pull request Sep 9, 2021

ZIP 239 support zcash/zcash#5291

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -1184,6 +1204,7 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pb
		LOCK(g_cs_recent_confirmed_transactions);
		for (const auto ptx : pblock->vtx) {

Use wtxid for transaction relay #18044

Use wtxid for transaction relay #18044

Conversation

sdaftuar commented Jan 31, 2020 • edited Loading

DrahtBot commented Jan 31, 2020 • edited Loading

Conflicts

sdaftuar commented Feb 4, 2020

jonatack commented Feb 4, 2020 • edited Loading

rajarshimaitra commented Feb 4, 2020

Choose a reason for hiding this comment

adamjonas commented Feb 6, 2020

ajtowns commented Feb 9, 2020

TheBlueMatt left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 13, 2020

fjahr commented Feb 14, 2020

instagibbs commented Feb 18, 2020

naumenkogs commented Feb 25, 2020

ariard commented Jul 23, 2020

Choose a reason for hiding this comment

ariard Jul 26, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin left a 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

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

sdaftuar commented Jan 31, 2020 •

edited

Loading

DrahtBot commented Jan 31, 2020 •

edited

Loading

jonatack commented Feb 4, 2020 •

edited

Loading

ariard Jul 26, 2020 •

edited

Loading