Remove ResendWalletTransactions from the Validation Interface #15632

jnewbery · 2019-03-20T22:16:59Z

Remove the Broadcast()/ResendWalletTransactions() notification from the Validation interface.

Closes #15619. See that issue for discussion.

jnewbery · 2019-03-20T22:18:08Z

Depends on #10973

jnewbery · 2019-03-20T22:22:38Z

Obviously requires careful testing. We don't currently have any automated testing for wallet rebroadcasts (wallet_resendwallettransactions.py() basically only tests the resendwallettransactions rpc method, and doesn't test whether:

ResendWalletTransactions() is called on a timer
ResendWalletTransactions() actually causes txs to be rebroadcast

jnewbery · 2019-03-20T22:23:31Z

Marking WIP until automated tests are written

DrahtBot · 2019-03-20T23:00:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
#15700 (rfc: Synchronize validation interface registration by promag)
#15639 (bitcoin-wallet tool: Drop libbitcoin_server.a dependency by ryanofsky)
#15638 (Move-only: Pull wallet code out of libbitcoin_server by ryanofsky)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)

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.

gmaxwell · 2019-03-21T06:21:57Z

This seems to add a lot of lines of code for something intended to be a simplification?

ryanofsky · 2019-03-21T07:10:49Z

This seems to add a lot of lines of code for something intended to be a simplification?

Only the last 3 commits are relevant here. The first 4 commits come from #10973 which would conflict with this PR, so John chose it as a base, but is actually independent. This PR could just as easily be based on master.

jnewbery · 2019-03-21T14:35:43Z

The first 4 commits come from #10973 which would conflict with this PR

Indeed. It didn't make sense to make or review changes in the node<->wallet interface while Russ's PR was open. It's merged now so I've rebased on master.

Changes are now +28/-35

jnewbery · 2019-03-22T19:16:15Z

Tests added. See #15646 . Removing WIP tag.

jnewbery · 2019-03-26T15:26:27Z

Rebased on the updated #15646 (please leave review comments for the test in that PR)

jnewbery · 2019-03-26T15:55:27Z

The test is currently unreliable for me. I think there's a bad interaction between using mocktime and the scheduler thread scheduling tasks.

ryanofsky

utACK 3e7bea5. I don't feel like I know CScheduler and SendMessages() code well enough to be extremely confident that this change doesn't break anything, but assuming CWallet::ResendWalletTransactions() is still called when it needs to be called, everything else looks correct to me and the changes are a nice simplification.

jamesob

Concept ACK!

maflcko

utACK 3e7bea5

SendMessages is called for every "active" peer after a sleep of 1000ms, so if the scheduler does it for the wallets once every 1000ms, that should still be more than sufficient.

maflcko · 2019-03-27T13:54:37Z

src/wallet/wallet.cpp

@@ -2138,8 +2138,13 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(interfaces::Chain::
    return result;
 }

-void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
+// Resend wallet transactions that haven't gotten in a block yet


Suggested change

// Resend wallet transactions that haven't gotten in a block yet

// Resend wallet transactions that haven't gotten in a block yet;

I'd just moved this comment from net_processing, but you're right that I should also improve it. Done!

maflcko · 2019-03-27T13:55:08Z

src/wallet/wallet.cpp

-void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
+// Resend wallet transactions that haven't gotten in a block yet
+// Except during reindex, importing and IBD, when old wallet
+// transactions become unconfirmed and spams other nodes.


Suggested change

// transactions become unconfirmed and spams other nodes.

// transactions become unconfirmed and spam other nodes.

promag

IIUC before this change ResendWalletTransactionsBefore could be called more frequently than once per second.

But considering

bitcoin/src/wallet/wallet.cpp

Lines 2152 to 2155 in 656a15e

    
           // Only do it if there's been a new block since last time 
        
           if (nBestBlockTime < nLastResend) 
        
               return; 
        
           nLastResend = GetTime();

It would wait for a new block to resend wallet transactions.

So I don't see a reason to not increase the scheduler period from 1 second to, for instance, 1 minute?

Edit: maybe the reason is to rush for the next block?

utACK 3e7bea5, I think this is a valid refactor: remove Broadcast signal from SendMessages and related cleanup.

maflcko · 2019-03-27T15:57:44Z

src/wallet/wallet.cpp

+    // Get the best block time
+    auto locked_chain = chain().lock();
+    Optional<int> tip_height = locked_chain->getHeight();
+    int64_t best_block_time = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;


On further thoughts on this, it seems brittle to use the time the miner decided to put in the block header as opposed to our local time, which at least can be assumed to be monotonic.

I think you're right that we should use the local time of the block received, but only in order to maintain existing behaviour.

Can you explain how using block time would be brittle?

It will hurt privacy even for tx that have a sufficient fee to get into the next block (+5 min). They will be rebroadcast regardless because a miner might have set the the block time to at least 5 minutes into the future.

Similarly, if the block time is set into the past, you might not rebroadcast transactions for quite some time, even if a block has been mined without your txs in the meantime.

I'm not really following how this is any worse for privacy, but I've reverted to using the local time of the tip being updated.

Not consistently worse, but in some race-conditions, one of which is enough to give an attacker useful observations

Attacker opens a bunch of new connections to you

You create tx with fee target of next block

tx is broadcast (attacker sees it)

less than 5 minutes pass, enough to mine a new block, but not enough to propagate the tx to miners

A block is mined with a timestamp of 60 min in the future, without your tx

Attacker sees the new block and opens a bunch of new connections to you

previous random wait in ResendWalletTransactions times out

we send the tx again to the attacker (we wouldn't do this for non-wallet txs, nor if we used local GetTime() for the block time)

promag · 2019-03-27T16:13:38Z

Could add UpdateBlockTip to interfaces::Chain::Notifications to avoid scheduler timer, maybe follow up?

jnewbery · 2019-03-27T16:16:21Z

Could add UpdateBlockTip to interfaces::Chain::Notifications to avoid scheduler timer, maybe follow up?

I don't understand this. How would this avoid using the schedule timer?

promag · 2019-03-27T16:41:12Z

@jnewbery untested, but see promag@afc7ac5.

jnewbery · 2019-03-27T17:12:36Z

Thanks @promag . Your change is definitely a simplification, but I think it makes the already bad privacy of rebroadcasts even worse.

Currently, wallet rebroadcasts are pretty bad for privacy: If you connect to a peer and they send you the same broadcast repeatedly, then it's their tx. Furthermore, they'll send all of their txs together, so it's even easier to identify which txs are rebroadcast.

The fact that we randomize when we rebroadcast transactions makes this slightly less bad. Your suggested change would be to only rebroadcast your wallet txs immediately after receiving a block. That would pretty obviously identify which txs are yours.

Ways we could improve rebroadcast privacy:

add a timer to each tx so that the rebroadcast schedules are independent
rotate peers more aggressively and only rebroadcast to new peers (so we only ever broadcast a tx to each peer once)

promag · 2019-03-27T17:28:53Z

I think I don't change the behavior?

jnewbery · 2019-03-27T17:32:21Z

I think I don't change the behavior?

Your change would mean that wallet txs are only ever rebroadcast immediately after a block is received. In the existing code, and in this PR, wallet txs are rebroadcast at random times.

…llet Summary: Move nTimeBestReceived (which is only used for wallet rebroadcasts) into the wallet. bitcoin/bitcoin@f463cd1 --- Depends on D5947 This is a partial backport of Core [[bitcoin/bitcoin#15632 | PR15632]] Test Plan: cmake .. -GNinja ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5948

Summary: Removes the now-unused Broadcast/ResendWalletTransactions interface from validationinterface. The wallet_resendwallettransactions.py needs a sleep added at the start to make sure that the rebroadcast scheduler is warmed up before the next block is mined. bitcoin/bitcoin@52b760f --- This is a partial backport of Core [[bitcoin/bitcoin#15632 | PR15632]] Test Plan: cmake .. -GNinja -DENABLE_WERROR=ON ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6031

…om ResendWalletTransactions Summary: bitcoin/bitcoin@833d98a --- This completes the backport of Core [[bitcoin/bitcoin#15632 | PR15632]] Test Plan: cmake .. -GNinja -DENABLE_WERROR=ON ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6032

7a9046e [wallet] Refactor CWalletTx::RelayWalletTransaction() (John Newbery) Pull request description: Refactor `CWalletTx::RelayWalletTransaction()` function. This was a suggestion from the wallet-node separation PR: bitcoin#15288 (comment), which we deferred until after the main PR was merged. There are also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed). ACKs for commit 7a9046: promag: utACK 7a9046e. MeshCollider: utACK bitcoin@7a9046e ryanofsky: utACK 7a9046e. No changes at all, just rebase after base PR bitcoin#15632 was merged Tree-SHA512: 2ae6214cfadd917a1b3a892c4277e5e57c3eb791e17f67511470e6fbc634d19356554b9f9c55af6b779fdef821914aad59b7cc9e6c13ece145df003bf507d486

…ion Interface

7a9046e [wallet] Refactor CWalletTx::RelayWalletTransaction() (John Newbery) Pull request description: Refactor `CWalletTx::RelayWalletTransaction()` function. This was a suggestion from the wallet-node separation PR: bitcoin#15288 (comment), which we deferred until after the main PR was merged. There are also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed). ACKs for commit 7a9046: promag: utACK 7a9046e. MeshCollider: utACK bitcoin@7a9046e ryanofsky: utACK 7a9046e. No changes at all, just rebase after base PR bitcoin#15632 was merged Tree-SHA512: 2ae6214cfadd917a1b3a892c4277e5e57c3eb791e17f67511470e6fbc634d19356554b9f9c55af6b779fdef821914aad59b7cc9e6c13ece145df003bf507d486

…ion Interface

…ation Interface

…ion Interface

jnewbery mentioned this pull request Mar 20, 2019

Remove Broadcast/ResendWalletTransactions from validation interface #15619

Closed

jnewbery changed the title ~~Remove ResendWalletTransactions from the Validation Interface~~ [WIP] Remove ResendWalletTransactions from the Validation Interface Mar 20, 2019

fanquake added Wallet Validation labels Mar 20, 2019

jnewbery force-pushed the no_resend_wallet_txs branch from 05af37e to 80f698a Compare March 21, 2019 14:35

jnewbery force-pushed the no_resend_wallet_txs branch from 80f698a to 012c7d2 Compare March 22, 2019 19:15

jnewbery changed the title ~~[WIP] Remove ResendWalletTransactions from the Validation Interface~~ Remove ResendWalletTransactions from the Validation Interface Mar 22, 2019

jnewbery force-pushed the no_resend_wallet_txs branch from 012c7d2 to 3e7bea5 Compare March 26, 2019 15:26

ryanofsky approved these changes Mar 26, 2019

View reviewed changes

jamesob reviewed Mar 26, 2019

View reviewed changes

maflcko reviewed Mar 27, 2019

View reviewed changes

promag reviewed Mar 27, 2019

View reviewed changes

maflcko reviewed Mar 27, 2019

View reviewed changes

jnewbery force-pushed the no_resend_wallet_txs branch from 3e7bea5 to 930cfbe Compare March 27, 2019 16:06

meshcollider merged commit 833d98a into bitcoin:master Apr 9, 2019

ryanofsky mentioned this pull request Apr 10, 2019

Move-only: Pull wallet code out of libbitcoin_server #15638

Merged

ryanofsky mentioned this pull request May 15, 2019

getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee #16028

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

66e0861

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

e36a2bc

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

4b02b18

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

e6e43af

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 3, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

4f51b54

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 4, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

0b3b18d

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 6, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

9f56f3c

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

32dce14

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

5a5432d

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

310002e

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 11, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

f53b0ab

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

318ebd4

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

partial bitcoin#15632: Remove ResendWalletTransactions from the Valid…

2fc48c9

…ation Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

partial bitcoin#15632: Remove ResendWalletTransactions from the Valid…

0179f5a

…ation Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

9c69438

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

02b0351

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

4bee847

…ion Interface

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15632: Remove ResendWalletTransactions from the Validat…

ed48a88

…ion Interface

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

	// Resend wallet transactions that haven't gotten in a block yet
	// Resend wallet transactions that haven't gotten in a block yet;

	// transactions become unconfirmed and spams other nodes.
	// transactions become unconfirmed and spam other nodes.

	// Only do it if there's been a new block since last time
	if (nBestBlockTime < nLastResend)
	return;
	nLastResend = GetTime();

Remove ResendWalletTransactions from the Validation Interface #15632

Remove ResendWalletTransactions from the Validation Interface #15632

Uh oh!

Conversation

jnewbery commented Mar 20, 2019

Uh oh!

jnewbery commented Mar 20, 2019

Uh oh!

jnewbery commented Mar 20, 2019

Uh oh!

jnewbery commented Mar 20, 2019

Uh oh!

DrahtBot commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

gmaxwell commented Mar 21, 2019

Uh oh!

ryanofsky commented Mar 21, 2019

Uh oh!

jnewbery commented Mar 21, 2019

Uh oh!

jnewbery commented Mar 22, 2019

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 27, 2019

Uh oh!

jnewbery commented Mar 27, 2019

Uh oh!

promag commented Mar 27, 2019

Uh oh!

jnewbery commented Mar 27, 2019

Uh oh!

promag commented Mar 27, 2019

Uh oh!

jnewbery commented Mar 27, 2019

Uh oh!

Uh oh!

DrahtBot commented Mar 20, 2019 •

edited

Loading

promag left a comment •

edited

Loading