Remove ResendWalletTransactions from the Validation Interface

jnewbery commented at 10:16 pm on March 20, 2019: member

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

Closes #15619. See that issue for discussion.

jnewbery commented at 10:18 pm on March 20, 2019: member

Depends on #10973

jnewbery commented at 10:22 pm on March 20, 2019: member

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 renamed this:
~~Remove ResendWalletTransactions from the Validation Interface~~
[WIP] Remove ResendWalletTransactions from the Validation Interface
on Mar 20, 2019

jnewbery commented at 10:23 pm on March 20, 2019: member

Marking WIP until automated tests are written

DrahtBot commented at 11:00 pm on March 20, 2019: member

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.

fanquake added the label Wallet on Mar 20, 2019

fanquake added the label Validation on Mar 20, 2019

gmaxwell commented at 6:21 am on March 21, 2019: contributor

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

ryanofsky commented at 7:10 am on March 21, 2019: member

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 force-pushed on Mar 21, 2019

jnewbery commented at 2:35 pm on March 21, 2019: member

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 force-pushed on Mar 22, 2019

jnewbery commented at 7:16 pm on March 22, 2019: member

Tests added. See #15646 . Removing WIP tag.

jnewbery renamed this:
~~[WIP] Remove ResendWalletTransactions from the Validation Interface~~
Remove ResendWalletTransactions from the Validation Interface
on Mar 22, 2019

jnewbery force-pushed on Mar 26, 2019

jnewbery commented at 3:26 pm on March 26, 2019: member

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

jnewbery commented at 3:55 pm on March 26, 2019: member

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

ryanofsky approved

ryanofsky commented at 7:00 pm on March 26, 2019: member

utACK 3e7bea52fa76d522ce5befb2546cab6959b7aed5. 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 commented at 7:10 pm on March 26, 2019: member

Concept ACK!

in src/wallet/wallet.cpp:2141 in 3e7bea52fa outdated

2137@@ -2138,8 +2138,13 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(interfaces::Chain::
2138     return result;
2139 }
2140 
2141-void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
2142+// Resend wallet transactions that haven't gotten in a block yet

MarcoFalke commented at 1:54 pm on March 27, 2019:

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

jnewbery commented at 4:06 pm on March 27, 2019:

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

in src/wallet/wallet.cpp:2143 in 3e7bea52fa outdated

2137@@ -2138,8 +2138,13 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(interfaces::Chain::
2138     return result;
2139 }
2140 
2141-void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
2142+// Resend wallet transactions that haven't gotten in a block yet
2143+// Except during reindex, importing and IBD, when old wallet
2144+// transactions become unconfirmed and spams other nodes.

MarcoFalke commented at 1:55 pm on March 27, 2019:

0// transactions become unconfirmed and spam other nodes.

jnewbery commented at 4:06 pm on March 27, 2019:

see above

MarcoFalke commented at 2:05 pm on March 27, 2019: member

utACK 3e7bea52fa76d522ce5befb2546cab6959b7aed5

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.

promag commented at 3:42 pm on March 27, 2019: member

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

But considering https://github.com/bitcoin/bitcoin/blob/656a15e5394d8d841619b5b186fa0f9c8be0322b/src/wallet/wallet.cpp#L2152-L2155 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.

in src/wallet/wallet.cpp:2160 in 3e7bea52fa outdated

2153@@ -2149,21 +2154,31 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, in
2154     if (fFirst)
2155         return;
2156 
2157+    // Get the best block time
2158+    auto locked_chain = chain().lock();
2159+    Optional<int> tip_height = locked_chain->getHeight();
2160+    int64_t best_block_time = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;

MarcoFalke commented at 3:57 pm on March 27, 2019:

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.

jnewbery commented at 4:15 pm on March 27, 2019:

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?

MarcoFalke commented at 6:23 pm on March 27, 2019:

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.

jnewbery commented at 7:09 pm on March 28, 2019:

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.

MarcoFalke commented at 9:04 pm on March 28, 2019:

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)

jnewbery force-pushed on Mar 27, 2019

promag commented at 4:13 pm on March 27, 2019: member

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

jnewbery commented at 4:16 pm on March 27, 2019: member

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 commented at 4:41 pm on March 27, 2019: member

@jnewbery untested, but see https://github.com/promag/bitcoin/commit/afc7ac58e9ee14d72467ae442256cf342f29ccfa.

jnewbery commented at 5:12 pm on March 27, 2019: member

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 commented at 5:28 pm on March 27, 2019: member

I think I don’t change the behavior?

jnewbery commented at 5:32 pm on March 27, 2019: member

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.

promag commented at 6:21 pm on March 27, 2019: member

@jnewbery I think you are missing this check:

0    // Only do it if there's been a new block since last time
1    if (best_block_time < nLastResend)
2        return;
3    nLastResend = GetTime();

so in the existing behavior (your change too) txs are broadcast only once shortly after a new block.

MarcoFalke commented at 6:25 pm on March 27, 2019: member

@promag , the nNextResend will be pushed into the future if there was not block

promag commented at 6:32 pm on March 27, 2019: member

@MarcoFalke right, but then if best_block_time is the same from the last ResendWalletTransactions then nothing happens.

promag commented at 6:48 pm on March 27, 2019: member

Sorry guys 😕it resends if “some random time passed AND there was a new block”.

DrahtBot added the label Needs rebase on Mar 27, 2019

in src/wallet/wallet.cpp:2126 in 930cfbe157 outdated

2137@@ -2138,8 +2138,13 @@ std::vector<uint256> CWallet::ResendWalletTransactionsBefore(interfaces::Chain::
2138     return result;
2139 }
2140 
2141-void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain, int64_t nBestBlockTime)
2142+// Resend unconfirmed wallet transactions
2143+void CWallet::ResendWalletTransactions()

ryanofsky commented at 8:27 pm on March 27, 2019:

Greg’s comments about this in IRC were very non-obvious to me. It would be good if the information could be added as a code comment:

http://www.erisian.com.au/bitcoin-core-dev/log-2019-03-27.html#l-219

<gmaxwell> jnewbery: the idea behind the current rebroadcast behavior is that unless the tip has changed we don’t have any reason to expect our transaction to have been mined, so the rebroadcast is pointless, since the txn might just be sitting waiting to be mined and all the rebroadcasts in the world won’t speed that up. <gmaxwell> (I mean the idea behind why it watches the tip) <gmaxwell> jnewbery: But thats still too rebroadcast heavy. If the tip changed but at our transaction wasn’t near the top of our mempool, well it shouldn’t have been mined then either. <gmaxwell> jnewbery: so right now if you make a txn that won’t get mined for 12 blocks, you’ll end up potentially rebroadcasting 12 times, even though it was in miners mempools the whole time. I’ve also seen network spies that appear to be attempting to exploit the rebroadcasting. <gmaxwell> So ideally we’d suppress rebroadcasting during periods when the txn is nowhere near the top. Rebroadcasting then is pointless and only serves to deanon us.

jnewbery commented at 9:23 pm on March 27, 2019: member

I intend to update this PR to make use of the UpdateBlockTip. As @MarcoFalke pointed out (https://github.com/bitcoin/bitcoin/pull/15632#discussion_r269639165), we should use the local time of the tip updated to maintain existing behaviour. To do that, the wallet should receive an UpdateBlockTip notification and store the local time (currently nTimeBestReceived is stored in validation, and should be removed by this PR).

jnewbery force-pushed on Mar 27, 2019

jnewbery commented at 9:31 pm on March 27, 2019: member

Rebased on master. I still intend to update this PR to use UpdateBlockTip (https://github.com/bitcoin/bitcoin/pull/15632#issuecomment-477353148) and add comments (https://github.com/bitcoin/bitcoin/pull/15632#discussion_r269754733)

DrahtBot removed the label Needs rebase on Mar 27, 2019

jnewbery force-pushed on Mar 28, 2019

jnewbery commented at 7:06 pm on March 28, 2019: member

New branch pushed. Changes:

don’t change to using tip block time. Continue using the local time of the tip being updated (plug wallet into UpdatedBlockTip notification and track with in the wallet)
add comment
add short sleep to test to make it run reliably (previously 10% of runs were failing because the block would be mined before the first time ResendWalletTransactions and so nNextResend was not being set.

Should be ready for review now.

MarcoFalke commented at 10:00 pm on March 28, 2019: member

nit in commit 94344f23ac2f914866f3f440e56dd4840344ff17:

Should also remove nTimeBestReceived in here, since the other changes make it unused? That way it is easier to see that they are the same value, just passed around differently.

Same goes for the next commit 82b2f8c5cf8bce0202c8911f6a57b97b87dc35a1:

The hunks that are removed in later commits should maybe already be removed in this commit. That way it is clearer that the code logic was moved and the only difference is the scheduling.

jnewbery commented at 11:58 pm on March 28, 2019: member

Thanks @MarcoFalke . I’ll rearrange commits tomorrow to aid review.

in src/wallet/wallet.h:661 in 5c79e30641 outdated

656@@ -657,6 +657,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
657     int64_t nNextResend = 0;
658     int64_t nLastResend = 0;
659     bool fBroadcastTransactions = false;
660+    // Local time that the tip block was received. Used to schedule wallet rebroadcasts.
661+    int64_t m_best_block_time {0};

ryanofsky commented at 5:30 pm on March 29, 2019:

This is accessed from two different threads so it might be better if this were atomic or accessed with the cs_wallet lock.

jnewbery commented at 6:29 pm on March 29, 2019:

Good catch! I’ll make this atomic.

in test/functional/wallet_resendwallettransactions.py:42 in 5c79e30641 outdated

38@@ -39,6 +39,10 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42+        # Sleep for just over a second so the rebroadcast scheduler can get

ryanofsky commented at 5:39 pm on March 29, 2019:

I don’t get why you’d need to sleep before a wait_until statement. Isn’t wait_until going to wait longer than 1.1 seconds for the condition to be true? It would be helpful to update this comment saying where the failure is and what happens if the sleep is not long enough.

jnewbery commented at 7:03 pm on March 29, 2019:

The wait_until() below can return in less than one second. If that happens then the rebroadcast timer won’t have popped for the first time, and so nNextResend won’t have been set. We then setmocktime, and when the rebroadcast timer does pop, nNextResend is still zero and ResendWalletTransactions() exits early.

ryanofsky approved

ryanofsky commented at 5:46 pm on March 29, 2019: member

utACK 5c79e306414be60323d1169136c261af0050fba9. Code changes look good but agree with Marco it could be nice to rearrange commits. Changes since last review: rebase, comment/test fixes, using UpdatedBlockTip time instead of block time.

jnewbery force-pushed on Mar 29, 2019

jnewbery commented at 7:04 pm on March 29, 2019: member

Commits updated as requested by @MarcoFalke and @ryanofsky.

in src/validationinterface.cpp:104 in b53a127ec0 outdated

100@@ -101,7 +101,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
101     conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1));
102     conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1));
103     conns.ChainStateFlushed = g_signals.m_internals->ChainStateFlushed.connect(std::bind(&CValidationInterface::ChainStateFlushed, pwalletIn, std::placeholders::_1));
104-    conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1, std::placeholders::_2));
105+    conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1));

MarcoFalke commented at 7:10 pm on March 29, 2019:

In commit b53a127ec02fa42e87cb4151e0529fbf3226187a:

Would have to adjust boost::signals2::signal<void (int64_t nBestBlockTime, CConnman* connman)> Broadcast; a few lines up as well

jnewbery commented at 7:58 pm on March 29, 2019:

Thanks. Fixed.

jnewbery force-pushed on Mar 29, 2019

DrahtBot added the label Needs rebase on Apr 1, 2019

jnewbery force-pushed on Apr 1, 2019

jnewbery commented at 8:48 pm on April 1, 2019: member

rebased

DrahtBot removed the label Needs rebase on Apr 1, 2019

DrahtBot added the label Needs rebase on Apr 2, 2019

jnewbery force-pushed on Apr 2, 2019

jnewbery commented at 3:20 pm on April 2, 2019: member

rebased

in src/wallet/wallet.cpp:2146 in f7790b3ad4 outdated

2142     nLastResend = GetTime();
2143 
2144     int relayed_tx_count = 0;
2145 
2146+    auto locked_chain = chain().lock();
2147     { // cs_wallet scope

MarcoFalke commented at 4:01 pm on April 2, 2019:

style-nit in f7790b3ad4c0cd5318841e30224c0d3dea0a3dca:

any reason to scope cs_wallet, but not cs_main?

jnewbery commented at 4:16 pm on April 2, 2019:

fixed

DrahtBot removed the label Needs rebase on Apr 2, 2019

in test/functional/wallet_resendwallettransactions.py:43 in f7790b3ad4 outdated

38@@ -39,6 +39,10 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42+        # Sleep for just over a second so the rebroadcast scheduler can get
43+        # warmed up (see fNextResend in ResendWalletTransactions())

MarcoFalke commented at 4:10 pm on April 2, 2019:

style-nit in commit 9ad047d936c5601ca3841303595368e3c23e50b9:

0        # warmed up (see nNextResend in ResendWalletTransactions(), which is called first scheduled 1sec after startup)

jnewbery commented at 4:16 pm on April 2, 2019:

fixed

MarcoFalke commented at 4:10 pm on April 2, 2019: member

utACK f7790b3ad4c0cd5318841e30224c0d3dea0a3dca

jnewbery force-pushed on Apr 2, 2019

jnewbery commented at 4:16 pm on April 2, 2019: member

Fixed @MarcoFalke’s review comments.

in test/functional/wallet_resendwallettransactions.py:42 in 0fb4ba63f3 outdated

38@@ -39,6 +39,11 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42+        # The rebroadcast scheduler can is first called 1 sec after startupp

MarcoFalke commented at 4:23 pm on April 2, 2019:

0        # The rebroadcast is first scheduled 1 sec after startup

in test/functional/wallet_resendwallettransactions.py:43 in 0fb4ba63f3 outdated

38@@ -39,6 +39,11 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42+        # The rebroadcast scheduler can is first called 1 sec after startupp
43+        # (see fNextResend in ResendWalletTransactions()). Sleep for just over

MarcoFalke commented at 4:23 pm on April 2, 2019:

0        # (see nNextResend in ResendWalletTransactions()). Sleep for just over

MarcoFalke commented at 4:23 pm on April 2, 2019: member

some typos in commit b1d1c414889aa2573806fe2d925651be66335bc4

jnewbery force-pushed on Apr 2, 2019

jnewbery commented at 4:28 pm on April 2, 2019: member

some typos

Sorry. Now fixed.

in test/functional/wallet_resendwallettransactions.py:44 in 4aa062760e outdated

38@@ -39,9 +39,9 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42-        # The rebroadcast scheduler can is first called 1 sec after startupp
43-        # (see fNextResend in ResendWalletTransactions()). Sleep for just over
44-        # a second to be certain that it's been called.
45+        # Wallet rebroadcast is first scheduled 1 sec after startup (see
46+        # nNextResend in ResendWalletTransactions()). Sleep for just over a
47+        # second to be certain that it has been called.

ryanofsky commented at 8:58 pm on April 2, 2019:

re: #15632 (review)

Maybe add “to be certain that it has been called before the first setmocktime call below”.

Normally when you see a sleep in a test, it’s inserted right before the thing that needs to be delayed, but in this case, the thing that needs to be delayed is further down, so it would be helpful to point out.

MarcoFalke commented at 9:04 pm on April 2, 2019: member

re-utACK 4aa062760e

in test/functional/wallet_resendwallettransactions.py:42 in 4aa062760e outdated

38@@ -39,9 +39,9 @@ def run_test(self):
39         self.log.info("Create a new transaction and wait until it's broadcast")
40         txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
41 
42-        # The rebroadcast scheduler can is first called 1 sec after startupp
43-        # (see fNextResend in ResendWalletTransactions()). Sleep for just over
44-        # a second to be certain that it's been called.
45+        # Wallet rebroadcast is first scheduled 1 sec after startup (see

ryanofsky commented at 9:10 pm on April 2, 2019:

In commit “[wallet] Remove unnecessary Chain::Lock parameter from ResendWalletTransactions” (4aa062760e5b6d91e3528833eff81d6a6cf96ff5)

Comment change should go in the previous commit which adds the comment and the sleep: “[wallet] Schedule tx rebroadcasts in wallet.” (b1d1c414889aa2573806fe2d925651be66335bc4), since this isn’t related to removing chain_lock.

ryanofsky approved

ryanofsky commented at 9:18 pm on April 2, 2019: member

utACK 4aa062760e5b6d91e3528833eff81d6a6cf96ff5. Changes since last review: rebase, dropping third commit and splitting it between the first two commits, moving new ResendWalletTransactions comment from 4th to 2nd commit, updating test comment, adding atomic type and cs_wallet lock

jnewbery force-pushed on Apr 2, 2019

jnewbery commented at 9:45 pm on April 2, 2019: member

@ryanofsky’s comments addressed. Sorry for the churn here!

ryanofsky approved

ryanofsky commented at 10:15 pm on April 2, 2019: member

utACK 28c75c0b5ece90caa1560c31d04a898587624ffe. Only change is tweaking the sleep comment in the second commit (and doing it there instead of the third commit)

MarcoFalke commented at 10:28 pm on April 2, 2019: member

re-utACK 28c75c0b5e

Only change is in a comment

MarcoFalke added this to the milestone 0.19.0 on Apr 2, 2019

in src/interfaces/chain.cpp:205 in dfe16f090e outdated

201@@ -202,16 +202,20 @@ class NotificationsHandlerImpl : public Handler, CValidationInterface
202     {
203         m_notifications->BlockDisconnected(*block);
204     }
205+    void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override

promag commented at 1:55 pm on April 3, 2019:

nits, space after *, snake case, and unused symbols.

in src/wallet/wallet.h:1239 in 3359591e30 outdated

1228@@ -1229,6 +1229,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
1229     friend struct WalletTestingSetup;
1230 };
1231 
1232+void MaybeResendWalletTxs();

promag commented at 1:58 pm on April 3, 2019:

nit, could have a comment.

in src/wallet/wallet.cpp:2155 in 28c75c0b5e outdated

2151@@ -2151,7 +2152,7 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain)
2152             // only rebroadcast unconfirmed txes older than 5 minutes before the
2153             // last block was found
2154             if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
2155-            relayed_tx_count += wtx.RelayWalletTransaction(locked_chain) ? 1 : 0;
2156+            relayed_tx_count += wtx.RelayWalletTransaction(*locked_chain) ? 1 : 0;

promag commented at 2:05 pm on April 3, 2019:

nit, if (wtx.RelayWalletTransaction(*locked_chain)) relayed_tx_count++; 🎉

in src/wallet/wallet.cpp:2157 in 28c75c0b5e outdated

2151@@ -2151,7 +2152,7 @@ void CWallet::ResendWalletTransactions(interfaces::Chain::Lock& locked_chain)
2152             // only rebroadcast unconfirmed txes older than 5 minutes before the
2153             // last block was found
2154             if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
2155-            relayed_tx_count += wtx.RelayWalletTransaction(locked_chain) ? 1 : 0;
2156+            relayed_tx_count += wtx.RelayWalletTransaction(*locked_chain) ? 1 : 0;
2157         }
2158     } // cs_wallet

promag commented at 2:07 pm on April 3, 2019:

Drop comment here and in L2145 (my preference), or update here.

promag commented at 2:08 pm on April 3, 2019: member

utACK, but my last comment should be addressed, the others are nits.

jnewbery commented at 2:36 pm on April 3, 2019: member

Thanks @promag . Comments addressed.

jnewbery force-pushed on Apr 3, 2019

promag commented at 2:46 pm on April 3, 2019: member

utACK 4a0d1d5, ty @jnewbery.

jnewbery force-pushed on Apr 3, 2019

MarcoFalke commented at 9:07 pm on April 3, 2019: member

re-utACK 10569b1067

jnewbery force-pushed on Apr 5, 2019

jnewbery commented at 2:21 pm on April 5, 2019: member

Removed extraneous ;.

promag commented at 6:13 pm on April 8, 2019: member

utACK fe82f31, only change is ; removal.

DrahtBot added the label Needs rebase on Apr 9, 2019

meshcollider commented at 12:47 pm on April 9, 2019: contributor

Apologies for forcing another rebase, LGTM though https://github.com/bitcoin/bitcoin/pull/15632/commits/fe82f317fad35fbe45caf84e224ec722e4cbd5ab

[wallet] Keep track of the best block time in the wallet

Move nTimeBestReceived (which is only used for wallet
rebroadcasts) into the wallet.

f463cd1073

ryanofsky approved

ryanofsky commented at 2:38 pm on April 9, 2019: member

utACK fe82f317fad35fbe45caf84e224ec722e4cbd5ab. No substantive changes since last review, just variable renames, formatting, new comments

[wallet] Schedule tx rebroadcasts in wallet

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.

52b760fc6a

[wallet] Remove unnecessary Chain::Lock parameter from ResendWalletTransactions 833d98ae07

jnewbery force-pushed on Apr 9, 2019

jnewbery commented at 2:40 pm on April 9, 2019: member

Apologies for forcing another rebase

Don’t apologise. This is what we all signed up for! Thanks for the review.

Rebased on master.

DrahtBot removed the label Needs rebase on Apr 9, 2019

ryanofsky approved

ryanofsky commented at 3:57 pm on April 9, 2019: member

utACK 833d98ae073daf0f25f786f043f2ffa85155c8ff. No changes, just rebase.

meshcollider merged this on Apr 9, 2019

meshcollider closed this on Apr 9, 2019

deadalnix referenced this in commit ecd9690042 on May 4, 2020

deadalnix referenced this in commit a044299f76 on May 11, 2020

deadalnix referenced this in commit 4920c60cc0 on May 11, 2020

vijaydasmp referenced this in commit c9be29bcbb on Oct 16, 2021

vijaydasmp referenced this in commit b2c2b9236e on Oct 16, 2021

kittywhiskers referenced this in commit 66e086157b on Nov 30, 2021

kittywhiskers referenced this in commit e36a2bcd8e on Nov 30, 2021

kittywhiskers referenced this in commit 4b02b18daa on Nov 30, 2021

kittywhiskers referenced this in commit e6e43af741 on Nov 30, 2021

kittywhiskers referenced this in commit 4f51b546dc on Dec 3, 2021

kittywhiskers referenced this in commit 0b3b18d3df on Dec 4, 2021

vijaydasmp referenced this in commit 6b539f8198 on Dec 5, 2021

vijaydasmp referenced this in commit a2b3ae0baf on Dec 6, 2021

vijaydasmp referenced this in commit 309825f547 on Dec 6, 2021

kittywhiskers referenced this in commit 9f56f3cf1e on Dec 6, 2021

kittywhiskers referenced this in commit 32dce144a7 on Dec 8, 2021

kittywhiskers referenced this in commit 5a5432dd28 on Dec 8, 2021

kittywhiskers referenced this in commit 310002e2f2 on Dec 8, 2021

kittywhiskers referenced this in commit f53b0ab8c2 on Dec 11, 2021

kittywhiskers referenced this in commit 318ebd4ff9 on Dec 12, 2021

kittywhiskers referenced this in commit 2fc48c9d4c on Dec 12, 2021

kittywhiskers referenced this in commit 0179f5a2bc on Dec 12, 2021

kittywhiskers referenced this in commit 9c69438a65 on Dec 12, 2021

kittywhiskers referenced this in commit 02b0351d64 on Dec 12, 2021

kittywhiskers referenced this in commit 4bee8477ef on Dec 12, 2021

kittywhiskers referenced this in commit ed48a889bf on Dec 12, 2021

MarcoFalke locked this on Dec 16, 2021

Remove ResendWalletTransactions from the Validation Interface #15632

Conflicts