Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from Apr 29, 2020

Conversation

amitiuttarwar
Copy link
Member

@amitiuttarwar amitiuttarwar commented Jan 31, 2020

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))

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jan 31, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #18807 ([WIP] unbroadcast follow-ups by amitiuttarwar)
  • #18781 (Add templated GetRandDuration<> by MarcoFalke)
  • #18764 (refactor: test: replace inv type magic numbers by constants by theStack)
  • #18446 (test: Add test for wtxid transaction relay by fjahr)
  • #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
  • #18044 (Use wtxid for transaction relay by sdaftuar)

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.

@amitiuttarwar amitiuttarwar force-pushed the 2020-01-unbroadcast branch 2 times, most recently from d229794 to faf5063 Compare Feb 5, 2020
@JeremyRubin JeremyRubin added this to Package Relay in Mempool improvements Feb 6, 2020
@JeremyRubin JeremyRubin moved this from Package Relay to Rebroadcasting in Mempool improvements Feb 6, 2020
MarcoFalke added a commit that referenced this issue Feb 18, 2020
8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar)
930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar)

Pull request description:

  This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.

  It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.

  This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.

ACKs for top commit:
  MarcoFalke:
    ACK 8bca30e, only change is some style fixups 🕓

Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Feb 18, 2020
8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar)
930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar)

Pull request description:

  This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.

  It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.

  This is currently used to support functional testing of the "unbroadcast" set tracking in bitcoin#18038. If this patch is accepted, it would also be useful to simplify the code in bitcoin#16698.

ACKs for top commit:
  MarcoFalke:
    ACK 8bca30e, only change is some style fixups 🕓

Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
@amitiuttarwar
Copy link
Member Author

@amitiuttarwar amitiuttarwar commented Feb 27, 2020

travis failure seems unrelated- feature_fee_estimation.py

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 27, 2020

It could be a crash, because node1 stops to log anything at all

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Concept ACK. Some design comments on the first commit 66ec557

src/txmempool.h Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
Copy link
Member

@naumenkogs naumenkogs left a comment

Concept ACK, reviewed code and left comments. Will test once we resolve the discussions.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
Copy link
Member

@MarcoFalke MarcoFalke left a comment

Some comments for the second commit

src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ariard ariard left a comment

If I understand well this PR, it introduces a initial-broadcast reattempt mechanism through mempool tracking and GETDATA monitoring. Compare to wallet rebroadcast, the safety catch of new one isn't confirmation-based but network-based so once you have reasonable propagation (reasonable=1) you stop initial-broadcast. But I don't grasp where the privacy win is in itself, I would say that's a reliability improvement against network unknowns (maybe even a slight worsening because now a spying node only have to be connected to you for REATTEMPT_BROADCAST_INTERVAL instead of ~1/30min, which decrease attacker deployment cost ?)

META: can you keep your commit message line to 74 characters :) ?

src/txmempool.h Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
@amitiuttarwar amitiuttarwar force-pushed the 2020-01-unbroadcast branch 2 times, most recently from 60dcb7c to 3d185ed Compare Mar 17, 2020
fanquake added a commit that referenced this issue May 21, 2020
…sanity check

651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069 [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](#18038 (comment)))
  - expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](#18038 (comment)))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](#18038 (comment)))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d8
  amitiuttarwar:
    ACK 651f1d8 🎉
  MarcoFalke:
    Review ACK 651f1d8

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
sidhujag pushed a commit to syscoin/syscoin that referenced this issue May 22, 2020
…empool sanity check

651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069 [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment)))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d8
  amitiuttarwar:
    ACK 651f1d8 🎉
  MarcoFalke:
    Review ACK 651f1d8

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this issue May 30, 2020
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
sidhujag pushed a commit to syscoin/syscoin that referenced this issue May 31, 2020
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
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
…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
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
…empool sanity check

651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069 [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment)))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d8
  amitiuttarwar:
    ACK 651f1d8 🎉
  MarcoFalke:
    Review ACK 651f1d8

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
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
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
…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
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
…empool sanity check

651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069 [wallet] remove nLastResend logic (gzhao408)

Pull request description:

  Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

  This PR addresses some of the outstanding TODOs building on top of it:
  - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment)))
  - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment)))

ACKs for top commit:
  naumenkogs:
    Code review ACK 651f1d8
  amitiuttarwar:
    ACK 651f1d8 🎉
  MarcoFalke:
    Review ACK 651f1d8

Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this issue Jun 10, 2020
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
MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this issue Jun 16, 2020
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
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jul 7, 2020
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
@ghost ghost mentioned this pull request Oct 20, 2020
sidhujag pushed a commit to syscoin-core/syscoin that referenced this issue Nov 10, 2020
8bca30e [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5 [lib] add scheduler to node context (Amiti Uttarwar)
930d837 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e8 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f6359 [util] allow scheduler to be mocked (Amiti Uttarwar)

Pull request description:

  This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.

  It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.

  This is currently used to support functional testing of the "unbroadcast" set tracking in bitcoin#18038. If this patch is accepted, it would also be useful to simplify the code in bitcoin#16698.

ACKs for top commit:
  MarcoFalke:
    ACK 8bca30e, only change is some style fixups 🕓

Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
MarcoFalke added a commit that referenced this issue Jan 5, 2021
7ff0535 [mempool] Remove error suppression on upgrade (Amiti Uttarwar)

Pull request description:

  In 0.21, we added unbroadcast txids to mempool.dat (#18038). When users upgraded from 0.21 to 0.22, this would throw a misleading "failed to deserialize mempool data" error even though everything actually loaded properly. So, commit 9c8a55d added a try-block to prevent throwing the error. After upgrading to 0.22, this exception handling is no longer useful, so now we can remove it.

ACKs for top commit:
  MarcoFalke:
    review ACK 7ff0535
  theStack:
    Code review ACK 7ff0535

Tree-SHA512: 0444eea2b1326904f9855fd0af6669a4990f0427cf7c9293252a5b7049cdcc785bdf9398fd08ed8dedacfdd78e75039ddf1087b3654c558ff52498df15f05daf
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jan 5, 2021
7ff0535 [mempool] Remove error suppression on upgrade (Amiti Uttarwar)

Pull request description:

  In 0.21, we added unbroadcast txids to mempool.dat (bitcoin#18038). When users upgraded from 0.21 to 0.22, this would throw a misleading "failed to deserialize mempool data" error even though everything actually loaded properly. So, commit 9c8a55d added a try-block to prevent throwing the error. After upgrading to 0.22, this exception handling is no longer useful, so now we can remove it.

ACKs for top commit:
  MarcoFalke:
    review ACK 7ff0535
  theStack:
    Code review ACK 7ff0535

Tree-SHA512: 0444eea2b1326904f9855fd0af6669a4990f0427cf7c9293252a5b7049cdcc785bdf9398fd08ed8dedacfdd78e75039ddf1087b3654c558ff52498df15f05daf
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 21, 2021
Summary:
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
  from a peer, or if the transaction is removed from the mempool.

PR 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/bitcoin#16698 | PR16698]].

This is a backport of Core [[bitcoin/bitcoin#18038 | PR18038]] [1/7]
bitcoin/bitcoin@89eeb4a

Test Plan: ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9006
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 21, 2021
Summary:
This is a backport of Core [[bitcoin/bitcoin#18038 | PR18038]] [2/7]
bitcoin/bitcoin@7e93eec
Depends on D9006

Test Plan: `ninja`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9007
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 21, 2021
Summary:
Every 10-15 minutes, the scheduler kicks off a job that queues unbroadcast
transactions onto each node.
This is a backport of Core [[bitcoin/bitcoin#18038 | PR18038]] [3/7]
bitcoin/bitcoin@e25e42f

Depends on D9007

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9008
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 21, 2021
Summary:
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)

This is a backport of Core [[bitcoin/bitcoin#18038 | PR18038]] [4/7]
bitcoin/bitcoin@dc1da48
Depends on D9008

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9009
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 21, 2021
Summary:
This is a backport of Core [[bitcoin/bitcoin#18038 | PR18038]] [5/7]
bitcoin/bitcoin@6851502
Depends on D9009

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9010
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.