Remove resendwallettransactions RPC method
This RPC was added for testing wallet rebroadcasts. Since we now have a real test for wallet rebroadcasts, it's no longer needed.
The call in wallet_basic.py can be removed because wallet_resendwallettransactions.py tests wallet rebroadcast.
2136 | + 2137 | + { // cs_wallet scope 2138 | + LOCK(cs_wallet); 2139 | + 2140 | + // Relay transactions 2141 | + for (std::pair<const uint256, CWalletTx>& item : mapWallet)
for (auto& item : mapWallet)
Style-nit: Since this isn't move only of the above method anyway, you are welcome to do style fixups (and clang-format, etc.)
ok, fixed this and some other style nits.
Concept ACK
I wasn't aware that this was a hidden RPC. Good to finally remove this privacy-footgun
Should deprecate first? Or that doesn't apply to hidden RPC?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
Should deprecate first? Or that doesn't apply to hidden RPC?
This RPC isn't part of the public API so I don't think it needs deprecation. We could raise a JSON RPC exception telling users that transactions are rebroadcast automatically and that they just need to wait.
The feature_pruning.py test was failing with this PR. I got bored of debugging a failing test case that took 30 minutes to run, so I resurrected #10591 here: #15686. That commit removes the resendwallettransactions call from feature_pruning.py.
Should deprecate first? Or that doesn't apply to hidden RPC?
This RPC isn't part of the public API so I don't think it needs deprecation. We could raise a JSON RPC exception telling users that transactions are rebroadcast automatically and that they just need to wait.
Then I think it's fine to just remove it.
Rebased on master and removed the [WIP] tag.
./test/functional/wallet_basic.py:10:1: F401 'test_framework.util.sync_mempools' imported but unused
This RPC was added for testing wallet rebroadcasts. Since we now have a
real test for wallet rebroadcasts, it's no longer needed.
The call in wallet_basic.py can be removed because
wallet_resendwallettransactions.py tests wallet rebroadcast.
2111 | @@ -2112,53 +2112,38 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const 2112 | return CTransaction(tx1) == CTransaction(tx2); 2113 | } 2114 | 2115 | -std::vector<uint256> CWallet::ResendWalletTransactionsBefore(interfaces::Chain::Lock& locked_chain, int64_t nTime)
I'm likely missing something, but should ResendWalletTransactionsBefore be removed from /src/wallet/wallet.h:950-951 as well?
// ResendWalletTransactionsBefore may only be called if fBroadcastTransactions!
std::vector<uint256> ResendWalletTransactionsBefore(interfaces::Chain::Lock& locked_chain, int64_t nTime);
You're not missing anything. Now fixed. Thanks!
2167 | + { // cs_wallet scope 2168 | + LOCK(cs_wallet); 2169 | + 2170 | + // Relay transactions 2171 | + for (std::pair<const uint256, CWalletTx>& item : mapWallet) 2172 | + {
I'd prefer if new code was properly clang-formatted
https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy
Fixed!
utACK f9b34f7cc2232dfd776885e20230ff3c660c2f7d
This is only called from ResendWalletTransactions(), so bring it inline.
utACK if you make @MarcoFalke happy.
re-utACK ea1a2d8794
Only whitespace changes since my previous review
utACK ea1a2d8.