Remove CWalletTx::AcceptToMemoryPool
Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay
Add a relay flag to broadcastTransaction because wasn’t sure of ReacceptWalletTransactions semantic.
Obviously, working on implementing #14978 (comment) to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
300@@ -306,10 +301,58 @@ class ChainImpl : public Chain
301 auto it = ::mempool.GetIter(txid);
302 return it && (*it)->GetCountWithDescendants() > 1;
303 }
304- void relayTransaction(const uint256& txid) override
305+ bool broadcastTransaction(const CTransactionRef& tx, const uint256& txid, CValidationState& state, const CAmount& absurdfee, bool relay) override
Thanks for consolidating the code from relayTransaction and submitToMemoryPool in interfaces/chain.cpp and from BroadcastTransaction in node/transaction.cpp.
I do think the right place for all this code to live is in node/transaction.cpp, though. interfaces/chain.cpp is really just meant to be a simple glue file with lines like:
0bool broadcastTransaction(...) override { return BroadcastTransaction(...); }
Also, I didn’t look closely yet, but it seems like this refactoring might be changing wallet behavior (this could explain travis test failures). It would probably be better if this PR avoided changing behavior to the extent possible. This might require adding some extra options to the existing BroadcastTransaction function in node/transaction.cpp, or adding a second broadcast function there that can hopefully share some code with the current one.
node/transaction.cpp was temporary and long term goal was to aggregate it to interfaces/node.cpp.
Didn’t want to change wallet behavior, fixed it by adding options in the existing BroadcastTransaction (but still not sure of ReacceptWalletTransactions, should we broadcast txn there ?), functional tests should be good now. Also rebased
re: #15713 (review)
Thanks! This looks good and now net removes code (+63/-83).
Okay was thinking than node/transaction.cpp was temporary
Yeah, that may have been the original thought. But I think it could be a reasonable long term location if more things are moved around it. In any case it’s where the BroadcastTransaction function currently lives, so it’s easier not to move in this PR, which is more about simplifying wallet code.
and long term goal was to aggregate it to interfaces/node.cpp
Everything in interfaces/, including interfaces/node.cpp is just supposed to be very short glue code (the files would be huge otherwise). The interfaces are a way of letting wallet, node, and gui code run in different processes, and a way of preventing them from accessing each others internal state.
204@@ -210,8 +205,7 @@ class NotificationsHandlerImpl : public Handler, CValidationInterface
205 // should be able to be removed entirely if `ResendWalletTransactions`
206 // is replaced by a wallet timer as suggested in
207 // https://github.com/bitcoin/bitcoin/issues/15619
1872- for (std::pair<const uint256, CWalletTx>& item : mapWallet)
1873 {
1874- const uint256& wtxid = item.first;
1875- CWalletTx& wtx = item.second;
1876- assert(wtx.GetHash() == wtxid);
1877+ auto locked_chain = chain().lock();
Thanks for looking at this @ariard. My view is different from @ryanofsky’s. I think rather than trying to maintain existing behaviour, we should use this interface tidyup to better define the relationship between the wallet and the node, ie to answer the question “What services should the node offer to the wallet?” Currently, the node exposes two methods to the wallet for sending a transaction:
submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state): Add transaction to memory pool if the transaction fee is below the amount specified by absurd_fee. Returns false if the transaction could not be added due to the fee or for another reason. This is only called in CWalletTx::AcceptToMemoryPool()relayTransaction(const uint256& txid): adds the txid to the INV queue for each peer. This is only called in CWalletTx::RelayWalletTransaction() (which also calls CWalletTx::AcceptToMemoryPool())Note that to relay an INV for the transaction to peers (2), the transaction must already be in the mempool. See:
relayTransaction() is only ever called if InMempool() || AcceptToMemoryPool() is true.
The wallet currently calls CWalletTx::AcceptToMemoryPool() in the following places:
1a. In CWallet::ReacceptWalletTransactions(), called after the wallet has started up to make sure that its transactions are added to the mempool at start.
2a. In CWalletTx::RelayWalletTransaction(), mentioned above.
3a. In CWallet::CommitTransaction(), when a transaction is to be broadcast to the network for the first time.
The wallet currently calls CWalletTx::RelayWalletTransaction() in the following places:
2a. In CWallet::ResendWalletTransactions(), called on a timer to rebroadcast unconfirmed transactions.
2b. In CWallet::CommitTransaction(), but only if the transaction is successfully added to the mempool.
I’ve been thinking about this in the context of the wallet rebroadcasting transactions (see discussion here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-03-29-19.10.log.html#l-9). My view now is that the wallet should not have any access to relay functionality. It should be able to submit transactions to the node’s mempool, and it should then be the mempool’s responsibility to broadcast/rebroadcast the transaction (see comment here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-03-29-19.10.log.html#l-92). That seems like a much cleaner divide of responsibility.
This PR seems like a simplification in terms of combining submitToMemoryPool() and relayTransaction() into a single method, but I don’t think that it moves us towards a point where the node<->wallet interface is simpler to understand.
John are you just saying that this PR does not go far enough, or that it does something bad or makes things more complicated?
I haven’t looked closely enough at this yet but if it exposes one node method node instead of two, and reduces the amount of code without changing behavior, it would seem like a strict improvement. Is your rebroadcast proposal harder to implement after this PR?
I haven’t looked closely enough at this yet but if it exposes one node method node instead of two, and reduces the amount of code without changing behavior, it would seem like a strict improvement.
It does reduce the number of node methods exposed, but does so by adding a flag to one of them which toggles the behaviour. I’m not sure if that counts as a simplification, or it’s just moving the complexity around. I’m also not sure if this maintains existing behaviour. For example, the GetDepthInMainChain(locked_chain) == 0 in RelayWalletTransaction() seems to have gone and not been replaced by anything.
Ariard or John, what are next steps? Should this PR be closed and revisited in the future if rebroadcasting is improved? Should the current code comment about this be removed or clarified?
I don’t want to discourage @ariard from making this change if you both think it’s an improvement over the existing code. It certainly doesn’t preclude us from makeing the changes that I proposed in #15713 (comment) at a later time. @ariard - if you think this should still go in, can you perhaps break up the PR into commits and justify any changes (eg why remove GetDepthInMainChain and IsCoinbase from RelayWalletTransaction().
Note that this conflicts with my #15728 which I think is a useful tidy-up in itself.
295@@ -301,10 +296,13 @@ class ChainImpl : public Chain
296 auto it = ::mempool.GetIter(txid);
297 return it && (*it)->GetCountWithDescendants() > 1;
298 }
299- void relayTransaction(const uint256& txid) override
300+ bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& absurdfee, bool wait_callback) override
wait_callback bool since it’s only ever called with wait_callback set to false.
300+ bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& absurdfee, bool wait_callback) override
301 {
302- CInv inv(MSG_TX, txid);
303- g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
304+ const TransactionError err = BroadcastTransaction(tx, txid, err_string, absurdfee, wait_callback);
305+ if (TransactionError::OK == err) {
return TransactionError::OK == err;
1906- // Don't relay coinbase transactions outside blocks
1907- if (IsCoinBase()) return false;
1908 // Don't relay abandoned transactions
1909 if (isAbandoned()) return false;
1910- // Don't relay conflicted or already confirmed transactions
1911- if (GetDepthInMainChain(locked_chain) != 0) return false;
HaveChain = !existingCoin.IsSpent() doesn’t mean if tx is in chain, but all of its outputs has been spent, we accept it again to mempool/relay ?
@ariard wrote:
I still have a doubt on BroadcastTransaction HaveChain check behavior.
HaveChain = !existingCoin.IsSpent()doesn’t mean if tx is in chain, but all of its outputs has been spent, we accept it again to mempool/relay ?
IsSpent() is “misleadingly” named. Because we use AccessCoin: Return a reference to Coin in the cache, or a pruned one if not found.. Pruned in this case means all its CTxOut are IsNull(), and IsSpent() just calls IsNull().
Thanks @Sjors . I agree with you that IsSpent() is a misleading name. A more accurate name would be DoesNotExist(). Negating the function here determines whether the coin exists in the current UTXO set.
The check in BroadcastTransaction() is an optimization: if any of the outputs from the tx still exist in the UTXO set then the tx must be confirmed. If the coin has been confirmed but it’s outputs don’t exist, then it’ll fail later (because its inputs can’t exist if it has already been spent).
This check in RelayWalletTransaction() is also an optimization. If the transaction has been confirmed (or a conflicting transaction has been confirmed), then the block hash will be recorded in the CMerkleTx, and we can check in the chain whether that block exists.
An alternative to removing this call to GetDepthInMainChain() would be to merge #15931 first, which removes GetDepthInMainChain’s requirement on locking the chain.
Thanks @Sjors . I agree with you that
IsSpent()is a misleading name. A more accurate name would beDoesNotExist(). Negating the function here determines whether the coin exists in the current UTXO set.The check in
BroadcastTransaction()is an optimization: if any of the outputs from the tx still exist in the UTXO set then the tx must be confirmed. If the coin has been confirmed but it’s outputs don’t exist, then it’ll fail later (because its inputs can’t exist if it has already been spent).
It might be useful to enhance the code documentation added in 976d1e485 with this information (if it isn’t expected to change soon). I found it v helpful to understanding the !existingCoin.IsSpent()) optimisation.
63 }
64
65 } // cs_main
66
67- promise.get_future().wait();
68+ if (wait_callback) {
callback_set{false}, setting it to true in the first else if (wait_callback) block, removing the second else if (wait_callback) block, and then testing for it here.
179@@ -189,8 +180,10 @@ class Chain
180 //! Check if transaction has descendants in mempool.
181 virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
182
183- //! Relay transaction.
184- virtual void relayTransaction(const uint256& txid) = 0;
185+ //! Transaction is added to memory pool, if the transdaction fee is below the
186+ //! amount specified by absurdfee, and broadcast to all nodes.
187+ //! Return false if the transaction could not be added due to the fee or for another reason.
188+ virtual bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& absurdfee, bool wait_callback) = 0;
max_tx_fee (rather than max_tx_fee, highfee, absurdfee, etc)
opinion submitting tx to memory pool should imply relay if it’s a valid one
No, this would cause the wallet to rebroadcast all of its unconfirmed transactions at startup. That’s a change in behaviour and a potentially bad privacy leak.
300+ bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
301 {
302- CInv inv(MSG_TX, txid);
303- g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
304+ const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_tx_fee, relay, false);
305+ return TransactionError::OK == err;
179@@ -189,8 +180,10 @@ class Chain
180 //! Check if transaction has descendants in mempool.
181 virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
182
183- //! Relay transaction.
184- virtual void relayTransaction(const uint256& txid) = 0;
185+ //! Transaction is added to memory pool, if the transdaction fee is below the
186+ //! amount specified by max_tx_fee, and broadcast to all nodes.
52@@ -52,6 +53,7 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx,
53 CallFunctionInValidationInterfaceQueue([&promise] {
54 promise.set_value();
55 });
56+ callback_set = true;
64@@ -63,7 +65,9 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx,
65
66 } // cs_main
67
68- promise.get_future().wait();
69+ if (callback_set) {
else block in LL60-64
15@@ -16,9 +16,11 @@
16 * @param[in] tx the transaction to broadcast
17 * @param[out] &txid the txid of the transaction, if successfully broadcast
18 * @param[out] &err_string reference to std::string to fill with error string if available
19- * @param[in] highfee Reject txs with fees higher than this (if 0, accept any fee)
20+ * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee)
21+ * @param[in] relay flag if both mempool insertion and p2p relay are requested
relay and are. I think two spaces at the front to align with the other params is fine.
35@@ -36,7 +36,6 @@
36 #include <validation.h>
37 #include <validationinterface.h>
38
39-
12@@ -13,10 +13,11 @@
13
14 #include <future>
15
16-TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee)
17+TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
relay bool but you’re not using it.
hashTx parameter from this interface method. The only caller of the method supplies an unused uint256 and then throws it away.
514@@ -515,11 +515,8 @@ class CWalletTx : public CMerkleTx
515
516 int64_t GetTxTime() const;
517
518- // Pass this transaction to the node to relay to its peers
519- bool RelayWalletTransaction(interfaces::Chain::Lock& locked_chain);
520-
521- /** Pass this transaction to the mempool. Fails if absolute fee exceeds absurd fee. */
522- bool AcceptToMemoryPool(interfaces::Chain::Lock& locked_chain, CValidationState& state);
523+ // SubmitMemoryPoolAndRelay may only be called if fBroadcastTransactions!
This comment is incorrect. The function will just return immediately if fBroadcastTransactions is set to false.
(I think this is from a bad rebase. The comment was changed to Pass this transaction to the node to relay to its peers in a more recent version.)
1890@@ -1892,30 +1891,27 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
1891 // Try to add wallet transactions to memory pool
1892 for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
1893 CWalletTx& wtx = *(item.second);
1894- CValidationState state;
1895- wtx.AcceptToMemoryPool(locked_chain, state);
1896+ std::string err_string;
1897+ wtx.SubmitMemoryPoolAndRelay(err_string);
1925+ // We must set fInMempool here - while it will be re-set to true by the
1926+ // entered-mempool callback, if we did not there would be a race where a
1927+ // user could call sendmoney in a loop and hit spurious out of funds errors
1928+ // because we think that this newly generated transaction's change is
1929+ // unavailable as we're not yet aware that it is in the mempool.
1930+ if (!(fInMempool |= pwallet->chain().broadcastTransaction(tx, txid, err_string, pwallet->chain().maxTxFee(), false))) return false;
chain().broadcastTransaction() could return false if BroadcastTransaction() returned TransactionError::P2P_DISABLED (ie the transaction is added to the mempool but not relayed).
This is also a behaviour change because previously, AcceptToMemoryPool() would return false if submitToMemoryPool() returned false. Now, if fInMempool is set to true but broadcastTransaction() returns false, then it’ll return true.
I think it makes sense to stick with the structure of the old AcceptToMemoryPool() function.
chain::relayTransaction(). I think it’s fine for the new broadcastTransaction interface method to only return false if the transaction failed to get into the mempool, and treat TransactionError::P2P_DISABLED as success. That way, fInMempool will get updated in exactly the same way as the existing code.
TransactionError::P2P_DISABLED == err check in broadcastTransaction interface
1890@@ -1892,30 +1891,27 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
1891 // Try to add wallet transactions to memory pool
1892 for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
1893 CWalletTx& wtx = *(item.second);
1894- CValidationState state;
1895- wtx.AcceptToMemoryPool(locked_chain, state);
1896+ std::string err_string;
unused_err_string to indicate that we throw it away without using it.
2154@@ -2160,9 +2155,10 @@ void CWallet::ResendWalletTransactions()
2155 // only rebroadcast unconfirmed txes older than 5 minutes before the
2156 // last block was found
2157 if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
2158- if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count;
2159+ std::string err_string;
unused_err_string to indicate that we throw it away without using it.
15@@ -16,9 +16,11 @@
16 * @param[in] tx the transaction to broadcast
17 * @param[out] &txid the txid of the transaction, if successfully broadcast
18 * @param[out] &err_string reference to std::string to fill with error string if available
19- * @param[in] highfee Reject txs with fees higher than this (if 0, accept any fee)
20+ * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee)
21+ * @param[in] relay flag if both mempool insertion and p2p relay are requested
22+ * @param[in] wait_callback, wait until callbacks have been processed to avoid stale result due to a sequentially RPC
wait_callback param that this must not be set while cs_main, cs_mempool or cs_wallet is being held or there’ll be a deadlock.
locked_chain is not defined in SubmitMemoryPoolAndRelay().
179@@ -189,8 +180,10 @@ class Chain
180 //! Check if transaction has descendants in mempool.
181 virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
182
183- //! Relay transaction.
184- virtual void relayTransaction(const uint256& txid) = 0;
185+ //! Transaction is added to memory pool, if the transdaction fee is below the
1897+ wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain);
1898 }
1899 }
1900
1901-bool CWalletTx::RelayWalletTransaction(interfaces::Chain::Lock& locked_chain)
1902+bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay, interfaces::Chain::Lock& locked_chain)
locked_chain as a parameter, even though it’s unused in this function in the final commit.
29@@ -30,6 +30,7 @@
30 #include <util/system.h>
31 #include <validation.h>
32 #include <validationinterface.h>
33+#include <node/transaction.h>
295@@ -301,10 +296,10 @@ class ChainImpl : public Chain
296 auto it = ::mempool.GetIter(txid);
297 return it && (*it)->GetCountWithDescendants() > 1;
298 }
299- void relayTransaction(const uint256& txid) override
300+ bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
301 {
302- CInv inv(MSG_TX, txid);
303- g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
304+ const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_tx_fee, relay, false);
nit: could include an inline comment for the last parameter:
0const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_tx_fee, relay, /*wait_callback*/ false);
68- promise.get_future().wait();
69+ if (callback_set) {
70+ promise.get_future().wait();
71+ }
72
73 if (!g_connman) {
if (relay) block? Returning a TransactionError::P2P_DISABLED error when the caller hasn’t asked for the transaction to be relayed seems wrong.
1920- pwallet->chain().relayTransaction(GetHash());
1921
1922- return true;
1923+ // Submit transaction to mempool for relay
1924+ pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString());
1925+ uint256 txid;
300+ bool broadcastTransaction(const CTransactionRef& tx, uint256& txid, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
301 {
302- CInv inv(MSG_TX, txid);
303- g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
304+ const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_tx_fee, relay, false);
305+ return TransactionError::OK == err || TransactionError::P2P_DISABLED == err;
This is exactly right, but I think it could do with a comment. Something like:
0// Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures
1// Note: this will need to be updated if BroadcastTransaction() is updated to return other non-mempool failures
2// that Chain clients do not need to know about.
a00303c updated with @jnewbery last review comments, specially removing unused uint256 from BroadcastTransaction interface
And “Initial build successful, but not enough time remains to run later build stages and tests. Please manually re-run this job by using the travis restart button or asking a bitcoin maintainer to restart. The next run should not time out because the build cache has been saved.”
Looks good. utACK e5b0fe904a9c6186b8273c61e93ee1538418b245
(I was only suggesting to remove the uint256 from the interface broadcastTransaction() method, not the BroadcastTransaction() function in node/transaction, but removing it there is also fine I think.)
Concept ACK on adding this as an interface method (I haven’t studied the rest). I helps me drop one commit from the hardware wallet support PR #14912.
It makes sense to filter some error messages in BroadcastTransaction, but for those that do make it through, at least the err_string - but probably also TransactionError - is useful for the client. So maybe it’s better to change TransactionError::OK and erase the error message for errors that we don’t want to propagate.
broadcastTransactions instead of a boolean and let caller to filter ?
You can filter it before returning, based on what you don’t want callers to see. But not based on what’s “used by no one right now”.
I think it’s fine to have the broadcastTransaction interface return what’s most useful to our wallet right now. This interface is meant to be private, so having it customized to our needs is ok, and if future changes to the wallet require the interface method to be changed to return more information, that’s also fine.
300+ {
301+ const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
302+ // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures.
303+ // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures
304+ // that Chain clients do not need to know about.
305+ return TransactionError::OK == err || TransactionError::P2P_DISABLED == err;
Might be better to do:
0switch(err) {
1 case TransactionError::OK:
2 case TransactionError::P2P_DISABLED:
3 return true;
4 case TransactionError::MISSING_INPUTS:
5 case TransactionError::ALREADY_IN_CHAIN:
6 case TransactionError::MEMPOOL_REJECTED:
7 case TransactionError::MEMPOOL_ERROR:
8 case TransactionError::INVALID_PSBT:
9 case TransactionError::PSBT_MISMATCH:
10 case TransactionError::SIGHASH_MISMATCH:
11 case TransactionError::MAX_FEE_EXCEEDED:
12 return false;
13}
That way you get a warning if there’s other values added that you haven’t handled.
75
76- CInv inv(MSG_TX, hashTx);
77- g_connman->ForEachNode([&inv](CNode* pnode) {
78- pnode->PushInventory(inv);
79- });
80+ if (relay) {
ACK c1351e28fd2e38a928b322456038198aa0e847bd
(but I don’t think it’s necessary to take that commit and invalidate @amitiuttarwar and @jonatack reviews)
ACK 976d1e4858df00c031ef2c07790862948dbc04cf, all tests pass, local git range-diff shows no change in previously-ACKed commits.
New commit makes doc-only changes in src/node/transaction.h and cleans up the BroadcastTransaction logic in src/node/transaction.cpp + improves code documentation.
I found #15713 (review) helpful in reviewing the last commit – this info could be added to the code documentation.
IIUC the logic changes in 976d1e4858 are
before
0if txn not in mempool and coin does not exist in the current UTXO set
1 submit txn
2else if coin exists in the current UTXO set
3 return "already in chain" txn error
4else
5 ensure we don't block if re-sending a transaction already in mempool
6end
now
0exit loop and return "already in chain" txn error if coin exists in the current UTXO set
1
2if txn not in mempool
3 submit txn
4end
EDIT: upgraded from partial ACK to ACK after re-review based on #15713 (review).
806@@ -807,14 +807,13 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
807 max_raw_tx_fee = fr.GetFee((weight+3)/4);
808 }
809
810- uint256 txid;
811 std::string err_string;
812- const TransactionError err = BroadcastTransaction(tx, txid, err_string, max_raw_tx_fee);
813+ const TransactionError err = BroadcastTransaction(tx, err_string, max_raw_tx_fee, true, true);
87cca293d5b127e43e35d866167893454658eacd
nit, could add a comment on the last 2 args.
80+ if (relay) {
81+ if (!g_connman) {
82+ return TransactionError::P2P_DISABLED;
83+ }
84+ CInv inv(MSG_TX, hashTx);
85+ g_connman->ForEachNode([&inv](CNode* pnode) {
13@@ -14,11 +14,13 @@
14 * Broadcast a transaction
15 *
16 * @param[in] tx the transaction to broadcast
17- * @param[out] &txid the txid of the transaction, if successfully broadcast
18 * @param[out] &err_string reference to std::string to fill with error string if available
19- * @param[in] highfee Reject txs with fees higher than this (if 0, accept any fee)
20+ * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee)
21+ * @param[in] relay flag if both mempool insertion and p2p relay are requested
22+ * @param[in] wait_callback, wait until callbacks have been processed to avoid stale result due to a sequentially RPC.
87cca293d5b127e43e35d866167893454658eacd
I think a better description/use-case is needed here.
25+ // and return early.
26 CCoinsViewCache &view = *pcoinsTip;
27- bool fHaveChain = false;
28- for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) {
29+ for (size_t o = 0; o < tx->vout.size(); o++) {
30 const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o));
IsSpent() doesn’t mean the coin is spent; it means the output doesn’t exist. So if the output does exist, then this transaction exists in the chain, i.e. it’s already confirmed.
Code review ACK 976d1e4.
As to why dropping if (GetDepthInMainChain(locked_chain) != 0) return false; is safe, IIUC because BroadcastTransaction loops through the transaction outputs and checks if they exist at the chain tip. By the way, the use of AccessCoin and IsSpent (actually meaning: “does not exist”) in BroadcastTransaction seems a needlessly confusing optimization.
I’m not sure if broadcastTransaction should suppress TransactionError::P2P_DISABLED.
From the old discussion with @jnewbery above I gather that this is the case because the old relayTransaction() didn’t have a return code, and so that we can update fInMempool conveniently.
However, the old CWalletTx::RelayWalletTransaction() had an explicite check for P2P (if (!pwallet->chain().p2pEnabled()) return false;) so if the node was not connected to the p2p network it would not call relayTransaction() and return false. Its replacement function SubmitMemoryPoolAndRelay doesn’t have this check and will return true in this case, so this looks like a change in behavior ( minor because its return value is only used for log messages as far as I can see).
Its replacement function SubmitMemoryPoolAndRelay doesn’t have this check and will return true in this case, so this looks like a change in behavior ( minor because its return value is only used for log messages as far as I can see).
Great catch @mzumsande! I agree that this is a slight change in behaviour, but that it’s only used for logging in ResendWalletTransactions() (The return value in CommitTransaction() should be the same as before).
p2pEnabled is just there when wallet is init before g_connman, and a quick look at init.cpp I’m not even sure that’s something possible. In future works, I’m thinking to remove p2pEnabled once we have the wallet registering its current height with Chain interface at handleNotifications. Wallet will only submit tx to the mempool and shouldn’t be aware of P2P state.
15+ * Submit a transaction to the mempool and (optionally) relay it to all P2P peers.
16+ *
17+ * Mempool submission can be synchronous (will await mempool entry notification
18+ * over the CValidationInterface) or asynchronous (will submit and not wait for
19+ * notification), depending on the value of wait_callback. wait_callback MUST
20+ * NOT be set while cs_main, cs_mempool or cs_wallet are held to avoid
AssertLockNotHeld?
sendrawtransaction, which is out of scope for this PR (but glad to address it in another PR if you think that’s relevant)
AssertLockNotHeld these locks too ?
2351@@ -2353,7 +2352,8 @@ void CWallet::ResendWalletTransactions()
2352 // only rebroadcast unconfirmed txes older than 5 minutes before the
2353 // last block was found
2354 if (wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;
2355- if (wtx.RelayWalletTransaction(*locked_chain)) ++relayed_tx_count;
2356+ std::string unused_err_string;
2357+ if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true, *locked_chain)) ++relayed_tx_count;
submitted_count ? Wallet is just submitting to mempool and that’s mempool job to pass it to P2P. We don’t dissociate errors here anyways
2150- pwallet->chain().relayTransaction(GetHash());
2151
2152- return true;
2153+ // Submit transaction to mempool for relay
2154+ pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString());
2155+ // We must set fInMempool here - while it will be re-set to true by the
Would be nice to update the comment here. This has been forgotten in 6ef86c92e7fcba866160d7a346fb260d7e4ab5bb
I guess you can just copy the commit body or something.
2134+bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay)
2135 {
2136 // Can't relay if wallet is not broadcasting
2137 if (!pwallet->GetBroadcastTransactions()) return false;
2138- // Don't relay coinbase transactions outside blocks
2139- if (IsCoinBase()) return false;
BroadcastTransaction does a convenience check that is only used in the sendrawtransaction (to notify a user of a transaction that is already confirmed). This is done by looking up if at least one of the outputs is in the utxo set. For the purpose of the wallet, this check is completely useless. Previously we wouldn’t call it and now we do (even for coinbase txs), where it can be trivially avoided.
I agree with Marco that this isn’t a layer violation. IsCoinBase() is a pure utility function and can be called anywhere.
But I also agree with ariard that this isn’t a big deal. Calling IsCoinBase() here is an optimization that affects a tiny number of transactions for a tiny number of users.
2139- // Don't relay coinbase transactions outside blocks
2140- if (IsCoinBase()) return false;
2141 // Don't relay abandoned transactions
2142 if (isAbandoned()) return false;
2143- // Don't relay conflicted or already confirmed transactions
2144- if (GetDepthInMainChain(locked_chain) != 0) return false;
LookupBlockIndex) with an utxo lookup. So going from memory to disk. Is this going to have a performance penalty?
AccessCoin in BroadcastTransaction. Isn’t CCoinsViewCache a in-memory cache? Is the wallet broadcasting that much transactions so it can be a performance penalty ? GetDepthInMainChain is also currently locking the chain via getBlockHeight
CCoinsViewCache hits disk if the in memory cache doesn’t contain the coin we’re looking for. I doubt the performance matters for the wallet, unlike e.g. peer to peer mempool relay.
ACK 976d1e4858df00c031ef2c07790862948dbc04cf, some questions
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 976d1e4858df00c031ef2c07790862948dbc04cf, some questions
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUiQegv/UAc28xrmuUqCZFUkvZtoLuw2Bq7qPa5zsh4Qr63OKNgh+BlzIh02YB3E
8l+8XvwKaPIhEk+flUc5KrBHpCe5kbRe0sY3X1FFVCI5ENTSKmm2+9U81ISgEAaZR
9nrZQL3kiwjSXN65k4lH3QfDgLJn5+LDZbW++Q/0NMFczMOPufEWZ168YXaZVnjpn
10Ts5cqHG4wfCdpbWVq8fFLxIYSKQEbu55+nheGG2dbDKu+qVSxt1J2+1NnAsVWHmh
11PTaUg9tmZqtpZuagHO7wGZj2Iwk7XZFjwi2htbKqoHjrOlov21FFZkcslAgc6Go4
12tvz0S0Bo5Y2mGRJ4wrioTfoDs0WFci2zBFdE1EQ11+hSuDGTcny4CKcnSFmlRWlc
13BD/34jM+55HM0okGs8lk6USeDsC7pn9Lzka6VOgM6D/N+O9nekimQvACdtY4/FHl
145fFiZp4Jc6hiYMIqyJsy+ooFufQTX6pVvGBrk7OVBE9GvCWFLAbv+arfO+1z4UUU
15uk0WUV1R
16=wLf3
17-----END PGP SIGNATURE-----
Timestamp of file with hash 27c1d332b653a5bb053c1a397b8772ee22e02d0e8091c6dfbe094774aaab128f -
See a patch (on top of master and this pull) for what I was thinking:
0001-Check-utxo-db-early-only-when-requested-in-Broadcast.patch.txt
284@@ -290,10 +285,13 @@ class ChainImpl : public Chain
285 auto it = ::mempool.GetIter(txid);
286 return it && (*it)->GetCountWithDescendants() > 1;
287 }
288- void relayTransaction(const uint256& txid) override
289+ bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
290 {
291- CInv inv(MSG_TX, txid);
292- g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
293+ const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false, /*check_chain*/ false);
Sorry, I think I got this wrong in my patch
0 const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*check_chain*/ false, /*wait_callback*/ false);
Okay pushed with your patch amended to invert comment on 4441ba6c.
More I looking at BroadcastTransaction, more I think it needs a PR of its own, maybe I can take your commit here and try to simplify more things?
I don’t like Marco’s suggested change. It’s leaking implementation detail to the caller and complicating the function interface. I also don’t understand the problem it’s supposed to be solving - is it just a performance optimization so that we don’t do the UTXO check for wallet-submitted transactions?
This PR had a bunch of ACKs without that change. Does it need to be included before this PR can be merged?
This PR had a bunch of ACKs without that change. Does it need to be included before this PR can be merged?
No, but I’d like to hear other’s opinion on it.
Also, you have just requested the conflicting pull to be merged, so this needs re-ACK anyway:
Rebased at 5922315 with #15713 (review), #15713 (review), #15713 (review), #15713 (review), #15713 (review) addressed. Shouldn’t have functional changes since 976d1e4.
Please review again, and let give your opinion on Marco patch : #15713 (comment).
You seem to have lost the if (relay) conditional in BroadcastTransaction(). I think that’s a rebase error.
You’ve also lost the g_connman error checking block:
0 if (!g_connman) {
1 return TransactionError::P2P_DISABLED;
2 }
Alternatively, you could assert that g_connman is non-null at the top of this function. It’s only called by RPCs and the wallet scheduler, both of which are initialized after g_connman is assigned and stopped before g_connman is reset.
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
Higher wallet-tx method combining RelayWalletTransactions and
AcceptToMemoryPool, using new Chain::broadcastTransaction
IsCoinBase check is already performed early by
AcceptToMemoryPoolWorker
GetDepthInMainChain check is already perfomed by
BroadcastTransaction
To avoid deadlock we MUST keep lock order in
ResendWalletTransactions and CommitTransaction,
even if we lock cs_main again further.
in BroadcastTransaction. Lock order will need
to be clean at once in a future refactoring
Oooops, thanks to pointing me the error, remove the check to be replaced by an assertion as it’s less checks in BroadcastTransaction at fb62f12.
Note : currently P2P_DISABLED error is still there even if it’s not used. It can be clean in a later commit if we agree that disabling P2P is not a thing.
re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgBCAv/bnyyRCCqUH8PSxg2lces1K5Z5M/Nb+eT5O2JQhQb1EfY+NckIcAqPmZz
8ffhn6qmibcfEUWVnhBdGD/8xZOdUM37IPs1xCVFt7kYdTIDkLGKKZqg9z9R14CZd
9aivpQia09e6zURD6a/d7qB8isfRCTNe3a29dcZv43oW2+WIStQGRBGswymI17k7C
10fYPsfWkAIru83VnoPCeziwXR85mRTXmKfVSQbgM/Qned2NDGGqZRZ6YH8KdtyPpP
11e3p0EAYMB+6aXum3Ky+u5gu9yRbx5pjBebBzQKbHHFHMLXxIBKAHml6qK7zqiO87
12Sg5fSDEQ/NC2DFfarRVWNT36UAhUqxpKwQptpWzMvgLXc2HYOR6MQrKM+eg/x84M
13YN5b9zlKXGySUWVZWqk+mL9ELUPDxZu6zRtzEK1TI8WXQi+bqqQrjV6rp8ZqGj+s
14M11E+efGRFL1qYOMEs0lHHcdqODkUoOJP1csVAGTDvFNJSDvbHMuqGUuObCHU8V1
15uqYjCCQV
16=XQET
17-----END PGP SIGNATURE-----
Timestamp of file with hash 9cd92754a615d69deebd843e2a0522965f52b1cf18a0916f147bb00a450d5790 -
67
68 } // cs_main
69
70- promise.get_future().wait();
71-
72- if (!g_connman) {
31- bool fHaveChain = false;
32- for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) {
33+ for (size_t o = 0; o < tx->vout.size(); o++) {
34 const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o));
35- fHaveChain = !existingCoin.IsSpent();
36+ // IsSpent doesnt mean the coin is spent, it means the output doesnt' exist.
doesn't twice.
re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
Let’s not bother with the check_chain patch 4441ba6cd940cf70b59182373cefeb28c22da4be (it’s not in the current version of this PR).
285@@ -291,9 +286,13 @@ class ChainImpl : public Chain
286 auto it = ::mempool.GetIter(txid);
287 return it && (*it)->GetCountWithDescendants() > 1;
288 }
289- void relayTransaction(const uint256& txid) override
290+ bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
291 {
292- RelayTransaction(txid, *g_connman);
293+ const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
294+ // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures.
BroadcastTransaction() can return are mempool-related failures.
13@@ -14,26 +14,30 @@
14
15 #include <future>
16
17-TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee)
18+TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
19 {
20+ assert(g_connman);
g_connman must always be assigned here.
post-merge ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab.
A couple of nits inline that could be cleaned up in a subsequent PR.