[doc / test / mempool] unbroadcast follow-ups #18807

amitiuttarwar · 2020-04-28T21:36:37Z

This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in LoadMempool when you upgrade software versions.

#18895 is another follow up to that addresses other functionality updates.

Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a GETDATA is received or the transaction is removed from the mempool.

DrahtBot · 2020-04-29T02:42:12Z

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

Conflicts

No conflicts as of last run.

…mprove wallet privacy 50fc4df [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar) 297a178 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar) 6851502 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar) dc1da48 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar) e25e42f [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar) 7e93eec [util] Add method that returns random time in milliseconds (Amiti Uttarwar) 89eeb4a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar) Pull request description: This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win. The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan. This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network. For privacy improvements around # 1, please see #16698. Thank you to gmaxwell for the idea of how to break out this subset of functionality (#16698 (comment)) ACKs for top commit: fjahr: Code review ACK 50fc4df MarcoFalke: ACK 50fc4df, I think this is ready for merge now 👻 amitiuttarwar: The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits. jnewbery: utACK 50fc4df. ariard: Code Review ACK 50fc4df (minor points no need to invalid other ACKs) robot-visions: ACK 50fc4df sipa: utACK 50fc4df naumenkogs: utACK 50fc4df Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f

jonatack

Quick first pass, will review after rebase.

doc/release-notes.md

test/functional/wallet_resendwallettransactions.py

jnewbery

Concept ACK. Will review fully once this is rebased.

src/net_processing.cpp

…ns to improve wallet privacy 50fc4df [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar) 297a178 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar) 6851502 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar) dc1da48 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar) e25e42f [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar) 7e93eec [util] Add method that returns random time in milliseconds (Amiti Uttarwar) 89eeb4a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar) Pull request description: This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win. The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan. This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network. For privacy improvements around # 1, please see bitcoin#16698. Thank you to gmaxwell for the idea of how to break out this subset of functionality (bitcoin#16698 (comment)) ACKs for top commit: fjahr: Code review ACK 50fc4df MarcoFalke: ACK 50fc4df, I think this is ready for merge now 👻 amitiuttarwar: The current tip `50fc4df` currently has 6 ACKs on it, so I've opened bitcoin#18807 to address the last bits. jnewbery: utACK 50fc4df. ariard: Code Review ACK 50fc4df (minor points no need to invalid other ACKs) robot-visions: ACK 50fc4df sipa: utACK 50fc4df naumenkogs: utACK 50fc4df Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f

fjahr · 2020-04-29T16:34:01Z

Concept ACK

brakmic · 2020-04-30T16:47:21Z

Concept ACK.

Built, run and tested the code on macOS Catalina 10.15.4. Both tests were successful.
Commit 0dd03ca

./test/functional/mempool_unbroadcast.py   (13s 854ms) │
2020-04-30T16:35:16.018000Z TestFramework (INFO): Initializing test directory /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_ngy9nfi0
2020-04-30T16:35:19.438000Z TestFramework (INFO): Test that mempool reattempts delivery of locally submitted transaction
2020-04-30T16:35:19.994000Z TestFramework (INFO): Generate transactions that only node 0 knows about
2020-04-30T16:35:21.479000Z TestFramework (INFO): Reconnect nodes & check if they are sent to node 1
2020-04-30T16:35:25.579000Z TestFramework (INFO): Add another connection & ensure transactions aren't broadcast again
2020-04-30T16:35:27.753000Z TestFramework (INFO): Test that transactions removed from mempool are removed from unbroadcast set
2020-04-30T16:35:28.071000Z TestFramework (INFO): Stopping nodes
2020-04-30T16:35:28.595000Z TestFramework (INFO): Cleaning up /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_ngy9nfi0 on exit
2020-04-30T16:35:28.596000Z TestFramework (INFO): Tests successful

./test/functional/wallet_resendwallettransactions.py  (13s 961ms)  
2020-04-30T16:36:05.847000Z TestFramework (INFO): Initializing test directory /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_k0jcybzc
2020-04-30T16:36:07.329000Z TestFramework (INFO): Create a new transaction and wait until it's broadcast
2020-04-30T16:36:17.090000Z TestFramework (INFO): Create a block
2020-04-30T16:36:17.213000Z TestFramework (INFO): Bump time & check that transaction is rebroadcast
2020-04-30T16:36:17.584000Z TestFramework (INFO): Stopping nodes
2020-04-30T16:36:18.055000Z TestFramework (INFO): Cleaning up /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_k0jcybzc on exit
2020-04-30T16:36:18.055000Z TestFramework (INFO): Tests successful

Will do a full review after rebase.

Sjors · 2020-05-04T18:53:56Z

the node will try to announce unbroadcast transactions until a peer requests it via a GETDATA

If I was a bully node, I would send a GETDATA back to any node who announces a transaction I haven't seen yet. Is that kind of behaviour punished at the p2p level? Would it make sense to require that the GETDATA comes from a node that we never told about the transaction? Or that it comes from at least two nodes?

amitiuttarwar · 2020-05-06T03:28:44Z

@gzhao408 has kindly taken over the follow-ups that change the codebase functionality in #18895 .

I'll use this PR to focus on updates to tests & docs. but currently still WIP.

@Sjors

If I was a bully node, I would send a GETDATA back to any node who announces a transaction I haven't seen yet. Is that kind of behaviour punished at the p2p level?

nodes requesting announced txns they haven't seen is ... desired :) but I think what you're getting at is the attacker might request but then not propagate. I don't see a way a peer could directly know if the other node propagated the transaction. So yes, we'd have to use another heuristic to try to evaluate that question.

but something to keep in mind.. transactions on the unbroadcast set are queued for every peer (ReattemptInitialBroadcast calls RelayTransaction which iterates through each node). so the attacker sending back a GETDATA would also have to somehow interrupt me from sending out to the other peers, be my only connection, etc.

Would it make sense to require that the GETDATA comes from a node that we never told about the transaction?

we don't usually get unsolicited GETDATA messages, but maybe what you're considering here is getting an INV from a peer we didn't announce it to. I considered this sort of approach in earlier iterations of the design (announce to some peers, wait for it to come back from others), but it seems unnecessary & a significant change to current txn propagation.

Or that it comes from at least two nodes?

also considered this approach, but over the course of review comments it didn't seem necessary bc some of the reasons talked about above. all of this design discussion is scattered throughout #18038 if you're interested in digging deeper :)

amitiuttarwar · 2020-05-15T01:23:44Z

test/functional/wallet_resendwallettransactions.py

        node.syncwithvalidationinterfacequeue()
-        node.p2ps[1].sync_with_ping()
+        now = int(time.time())


@MarcoFalke I removed the sync_with_ping() call because I think its a no-op, but I'd like your review to make sure I'm not (re)introducing a race in weird conditions.

my understanding:
sync_with_ping() is a p2p interface method that ensures any messages it has sent to the node have been processed by the node.

the syncwithvalidationinterfacequeue() makes sense to me bc that's making sure the wallet gets notified about the block.

since we're trying to check if the wallet kicks off rebroadcasting the transaction, so this synchronization should not be relevant? it was definitely not sufficient. the test previously wasn't checking anything bc it didn't give enough time for a rebroadcast to kickoff, so I've added a time.sleep() call.

tldr; I removed the sync_with_ping() bc I think it was a no-op & misleading. I've added a time.sleep(2) to actually test that a transaction isn't rebroadcast. please lmk if I'm missing something.

sync_with_ping() is a p2p interface method that ensures any messages it has sent to the node have been processed by the node.

Correct. Also, any messages that the node has put in the send buffer (before the pong is sent) will get "flushed out" to the mininode.

so, do you agree that seems irrelevant here?

Returning by const value is only meaningful in a specific circumstance around user defined types. In this case, the const is not enforcing any restrictions on the call site, so is misleading.

amitiuttarwar · 2020-05-25T18:34:08Z

thanks for review @MarcoFalke & @jonatack, comments have been addressed !

glozow

ACK 9e1cb1a
And thank you for touching things up for me :)

maflcko · 2020-05-30T16:21:51Z

ACK 9e1cb1a 👁

Show signature and timestamp

Signature:

Timestamp of file with hash de6ba48e94c3bcb5032ea5569ce708038f02ef67193d1448110f51b57e5d3132 -

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a 👁 gzhao408: ACK [`9e1cb1a`](bitcoin@9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

glowang · 2020-06-05T15:41:33Z

test/functional/wallet_resendwallettransactions.py

+        # Transaction should not be rebroadcast within first 12 hours
+        # Leave 2 mins for buffer
+        twelve_hrs = 12 * 60 * 60
+        two_min = 2 * 60


I'm curious - any reason why we chose 2 minutes here?

It can probably be reduced to one second, if now gets set to block_time (the mock time before this call)

…ns to improve wallet privacy 50fc4df [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar) 297a178 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar) 6851502 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar) dc1da48 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar) e25e42f [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar) 7e93eec [util] Add method that returns random time in milliseconds (Amiti Uttarwar) 89eeb4a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar) Pull request description: This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win. The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan. This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network. For privacy improvements around # 1, please see bitcoin#16698. Thank you to gmaxwell for the idea of how to break out this subset of functionality (bitcoin#16698 (comment)) ACKs for top commit: fjahr: Code review ACK 50fc4df MarcoFalke: ACK 50fc4df, I think this is ready for merge now 👻 amitiuttarwar: The current tip `50fc4df` currently has 6 ACKs on it, so I've opened bitcoin#18807 to address the last bits. jnewbery: utACK 50fc4df. ariard: Code Review ACK 50fc4df (minor points no need to invalid other ACKs) robot-visions: ACK 50fc4df sipa: utACK 50fc4df naumenkogs: utACK 50fc4df Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a eye gzhao408: ACK [`9e1cb1a`](9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

…ns to improve wallet privacy 50fc4df [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar) 297a178 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar) 6851502 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar) dc1da48 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar) e25e42f [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar) 7e93eec [util] Add method that returns random time in milliseconds (Amiti Uttarwar) 89eeb4a [mempool] Track "unbroadcast" transactions (Amiti Uttarwar) Pull request description: This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win. The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan. This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network. For privacy improvements around # 1, please see bitcoin#16698. Thank you to gmaxwell for the idea of how to break out this subset of functionality (bitcoin#16698 (comment)) ACKs for top commit: fjahr: Code review ACK 50fc4df MarcoFalke: ACK 50fc4df, I think this is ready for merge now 👻 amitiuttarwar: The current tip `50fc4df` currently has 6 ACKs on it, so I've opened bitcoin#18807 to address the last bits. jnewbery: utACK 50fc4df. ariard: Code Review ACK 50fc4df (minor points no need to invalid other ACKs) robot-visions: ACK 50fc4df sipa: utACK 50fc4df naumenkogs: utACK 50fc4df Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a eye gzhao408: ACK [`9e1cb1a`](9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

16d4b3f test: mempool.dat compatibility between versions (Ivan Metlushko) Pull request description: Rationale: Verify mempool.dat compatibility between versions The format of mempool.dat has been changed in bitcoin#18038 The tests verifies the fix made in bitcoin#18807 and ensures that the file format is compatible between current version and v0.19.1 The test verifies both backward and forward compatibility. This PR also adds a log when we fail to add a tx loaded from mempool.dat. It was useful when debugging this test and could be potentially useful to debug other scenarios as well. Closes bitcoin#19037 ACKs for top commit: Sjors: tACK 16d4b3f Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490

Summary: Check that... - mempool tracks & reattempts delivery of a transaction where a GETDATA hasn't been requested by a peer yet. - transaction delivery is not attempted again after GETDATA is received. - transaction is removed from the unbroadcast set when its removed from the mempool. --- Adapted to our different APIs: - create_confirmed_utxos is defined in test_framework.blocktools - disconnect_nodes takes a node as 2nd parameter, not a node index Includes a bugfix from [[bitcoin/bitcoin@bd093ca | bd093ca15]]: - add missing parentheses after `node.disconnect_p2ps` This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [6/7] bitcoin/bitcoin@297a178 Depends on D9010 Test Plan: `tests/functional/test_runner.py mempool_unbroadcast` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9011

Summary: Ensure that the unbroadcast set will still be meaningful if the node is restarted. This concludes backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [7/7] bitcoin/bitcoin@50fc4df Depends on D9011 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9012

Summary: This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [1/9] bitcoin/bitcoin@dab298d Note: the 3rd commit of this PR will not be backported, as it added a duplicate line later removed in [[bitcoin/bitcoin#19178 | PR19178]] Test Plan: proof-reading Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9028

Summary: - add () to function to actually disconnect from p2pconn - extract max interval into a constant - disconnect at the end of a subtest rather than start of next This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [2/9] bitcoin/bitcoin@bd093ca Test Plan: `test/functional/test_runner.py mempool_u*` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9029

Summary: This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [3/9] bitcoin/bitcoin@ba54983 Test Plan: test/functional/test_runner.py wallet_resendwallettransactions.py Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9030

Summary: mempool.dat now contains unbroadcast transactions, that did not exist in mempool.dat files created in previous version. This commit prevent an uneccessary error message to be logged when users update from v0.22.11 or lower. The try..catch can be removed after everyone has upgraded to > v0.22.11. This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [4/9] bitcoin/bitcoin@9c8a55d Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9031

Summary: This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [5/9] bitcoin/bitcoin@1f94bb0 Test Plan: ninja Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9032

Summary: there were two calls to disconnect_nodes that were no-ops. fixed one & removed the other & added assertions to confirm node has no connections when creating the unbroadcast transaction. This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] bitcoin/bitcoin@fa32e67 Test Plan: test/functional/test_runner.py mempool_persist Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9033

Summary: Returning by const value is only meaningful in a specific circumstance around user defined types. In this case, the const is not enforcing any restrictions on the call site, so is misleading. This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [7/9] bitcoin/bitcoin@750456d Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9034

Summary: This is a backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [8/9] bitcoin/bitcoin@8f30260 Test Plan: `src/bitcoin-cli help getrawmempool` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9035

Summary: This concludes backport of Core [[bitcoin/bitcoin#18807 | PR18807]] [9/9] bitcoin/bitcoin@9e1cb1a Test Plan: `ninja` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9036

amitiuttarwar mentioned this pull request Apr 28, 2020

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038

Merged

DrahtBot added Docs Mempool P2P Tests Validation Wallet labels Apr 28, 2020

amitiuttarwar force-pushed the 2020-01-unbroadcast-followup branch 2 times, most recently from 6fd8355 to 0dd03ca Compare April 29, 2020 02:10

DrahtBot added the Needs rebase label Apr 29, 2020

jonatack reviewed Apr 29, 2020

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

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

jnewbery reviewed Apr 29, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

amitiuttarwar force-pushed the 2020-01-unbroadcast-followup branch from 0dd03ca to 41fd9c1 Compare May 1, 2020 21:43

DrahtBot removed the Needs rebase label May 1, 2020

glozow mentioned this pull request May 6, 2020

p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check #18895

Merged

fanquake removed the Docs label May 6, 2020

amitiuttarwar force-pushed the 2020-01-unbroadcast-followup branch from 41fd9c1 to b9866b9 Compare May 15, 2020 00:53

amitiuttarwar commented May 15, 2020

View reviewed changes

amitiuttarwar added 3 commits May 25, 2020 11:27

[trivial] Remove misleading 'const'

750456d

Returning by const value is only meaningful in a specific circumstance around user defined types. In this case, the const is not enforcing any restrictions on the call site, so is misleading.

[doc] Update unbroadcast description in RPC results

8f30260

[trivial/doc] Fix comment type

9e1cb1a

amitiuttarwar force-pushed the 2020-01-unbroadcast-followup branch from 8660815 to 9e1cb1a Compare May 25, 2020 18:27

glozow reviewed May 25, 2020

View reviewed changes

maflcko merged commit 826fe9c into bitcoin:master May 30, 2020

S3RK mentioned this pull request Jun 3, 2020

test: mempool compatibility test #19153

Merged

glowang reviewed Jun 5, 2020

View reviewed changes

luke-jr mentioned this pull request Jul 11, 2020

Refactor mempool.dat to be extensible, and store missing info #19488

Closed

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

[doc / test / mempool] unbroadcast follow-ups #18807

[doc / test / mempool] unbroadcast follow-ups #18807

Uh oh!

Conversation

amitiuttarwar commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr commented Apr 29, 2020

Uh oh!

brakmic commented Apr 30, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

amitiuttarwar commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitiuttarwar May 15, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko May 15, 2020

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar May 15, 2020 • 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!

amitiuttarwar commented May 25, 2020

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 30, 2020

Uh oh!

glowang Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amitiuttarwar commented Apr 28, 2020 •

edited

Loading

DrahtBot commented Apr 29, 2020 •

edited

Loading

amitiuttarwar commented May 6, 2020 •

edited

Loading

amitiuttarwar May 15, 2020 •

edited

Loading