refactor: use RelayTransaction in BroadcastTransaction utility

ariard commented at 5:33 pm on July 24, 2019: member

Implementing suggestion in #15713 (review).

Seems a reason of these node utilities is to glue with already there functions, so we should reuse them.

DrahtBot commented at 5:59 pm on July 24, 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:

#16443 (refactor: have CCoins* data managed under CChainState by jamesob)
#15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
#15218 (validation: Flush state after initial sync by andrewtoth)

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.

MarcoFalke renamed this:
~~refactor : use RelayTransaction in BroadcastTransaction utility~~
refactor: use RelayTransaction in BroadcastTransaction utility
on Jul 24, 2019

MarcoFalke commented at 6:54 pm on July 24, 2019: member

re-run ci

MarcoFalke closed this on Jul 24, 2019

MarcoFalke reopened this on Jul 24, 2019

jnewbery commented at 7:05 pm on July 24, 2019: member

Concept ACK.

I had some suggested changes to BroadcastTransaction() here: https://github.com/jnewbery/bitcoin/commit/ecfeea015919cc44b465e969e65cb2c459c1235c (removing unnecessary local variables, improving comments). Feel free to take that commit if you think it makes sense to bundle it in this PR.

in src/net_processing.h:94 in ddab045202 outdated

89@@ -90,4 +90,7 @@ struct CNodeStateStats {
90 /** Get statistics from node state */
91 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
92 
93+/** Relay transaction to every node */
94+void RelayTransaction(const CTransaction& tx, CConnman* connman);

MarcoFalke commented at 7:07 pm on July 24, 2019:

style-nit: connman can be a const reference instead of a mutable raw pointer, since we don’t modify connman, but push the tx to the set of all nodes.

0void RelayTransaction(const CTransaction& tx, const CConnman& connman);

MarcoFalke commented at 7:14 pm on July 24, 2019:

style-nit: (feel free to ignore)

It is sufficient to pass the txid (as we assume the tx is in the mempool). So maybe make the signature similar to ChainImpl’s void relayTransaction(const uint256& txid) and use it there as well?

MarcoFalke approved

MarcoFalke commented at 7:08 pm on July 24, 2019: member

ACK ddab0452020bba2dcdd342437454cd01227d401e

DrahtBot added the label P2P on Jul 24, 2019

DrahtBot added the label Refactoring on Jul 24, 2019

promag commented at 7:41 pm on July 24, 2019: member

ACK, also Marco suggestions.

ariard force-pushed on Jul 24, 2019

ariard commented at 8:12 pm on July 24, 2019: member

@MarcoFalke addressed your comments but john commit makes more sense than I take it on top of #15713

ariard force-pushed on Jul 24, 2019

in src/net_processing.cpp:1310 in 3d35410014 outdated

1304@@ -1305,7 +1305,7 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1305     return true;
1306 }
1307 
1308-static void RelayTransaction(const CTransaction& tx, CConnman* connman)
1309+void RelayTransaction(const uint256& txid, CConnman* connman)
1310 {
1311     CInv inv(MSG_TX, tx.GetHash());

MarcoFalke commented at 9:13 pm on July 24, 2019:

0     CInv inv(MSG_TX, txid);

in src/net_processing.cpp:2484 in 3d35410014 outdated

2479@@ -2480,7 +2480,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2480         vRecv >> ptx;
2481         const CTransaction& tx = *ptx;
2482 
2483-        CInv inv(MSG_TX, tx.GetHash());
2484+
2485+        const uint256& hash = tx.GetHash();

MarcoFalke commented at 9:14 pm on July 24, 2019:

GetHash already returns a reference, so you don’t need this alias, I think

in src/net_processing.h:94 in 3d35410014 outdated

89@@ -90,4 +90,7 @@ struct CNodeStateStats {
90 /** Get statistics from node state */
91 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
92 
93+/** Relay transaction to every node */
94+void RelayTransaction(const CTransaction& tx, const CConnman& connman);

MarcoFalke commented at 9:15 pm on July 24, 2019:

0void RelayTransaction(const uint256& txid, const CConnman& connman);

MarcoFalke commented at 9:16 pm on July 24, 2019: member

re-ACK apart from compile failures

ariard force-pushed on Jul 24, 2019

ariard commented at 9:48 pm on July 24, 2019: member

Sorry for compilation failures, should be good now.

in src/node/transaction.cpp:73 in 46990d2c4b outdated

69@@ -69,10 +70,7 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx,
70         return TransactionError::P2P_DISABLED;
71     }
72 
73-    CInv inv(MSG_TX, hashTx);
74-    g_connman->ForEachNode([&inv](CNode* pnode) {
75-        pnode->PushInventory(inv);
76-    });
77+    RelayTransaction(hashTx, std::move(*g_connman));

MarcoFalke commented at 9:55 pm on July 24, 2019:

I don’t think you can move references

0    RelayTransaction(hashTx, *g_connman);

Also, what happened to this suggestion:

 0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 1index 02f39cef8e..22e4aaedaf 100644
 2--- a/src/interfaces/chain.cpp
 3+++ b/src/interfaces/chain.cpp
 4@@ -9,6 +9,7 @@
 5 #include <interfaces/handler.h>
 6 #include <interfaces/wallet.h>
 7 #include <net.h>
 8+#include <net_processing.h>
 9 #include <node/coin.h>
10 #include <policy/fees.h>
11 #include <policy/policy.h>
12@@ -292,8 +293,7 @@ public:
13     }
14     void relayTransaction(const uint256& txid) override
15     {
16-        CInv inv(MSG_TX, txid);
17-        g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
18+        RelayTransaction(txid, *g_connman);
19     }
20     void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override
21     {

ariard commented at 10:05 pm on July 24, 2019:

You can move reference but yes that’s useless, I drop it so.

Added suggestion, didn’t see it at first

ariard force-pushed on Jul 24, 2019

refactor : use RelayTransaction in BroadcastTransaction utility

To do so, we also refactor RelayTransaction to take a txid
instead of passing a tx

9bc8b28c1d

ariard force-pushed on Jul 24, 2019

in src/net_processing.cpp:1311 in 9bc8b28c1d

1309+void RelayTransaction(const uint256& txid, const CConnman& connman)
1310 {
1311-    CInv inv(MSG_TX, tx.GetHash());
1312-    connman->ForEachNode([&inv](CNode* pnode)
1313+    CInv inv(MSG_TX, txid);
1314+    connman.ForEachNode([&inv](CNode* pnode)

promag commented at 0:33 am on July 25, 2019:

µnit, pardon

0[&inv](CNode* pnode) {

MarcoFalke commented at 11:48 am on July 25, 2019:

I’d rather keep it the way it is now

promag commented at 0:37 am on July 25, 2019: member

ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9, verified there are no more PushInventory(CInv(MSG_TX, ..., nice refactor, :+1: @amitiuttarwar.

MarcoFalke commented at 11:55 am on July 25, 2019: member

ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjNYgv/ecqESlAxc75rOwly8VCGKZjH4Zdb0+lfb3LnFTNgsPkd2OT1UZ9t3qby
 8J18SGBradFqTrRIivGqwxnOkIKqc2HO+7g2M2NERzdudVE0TOauFIMC+R0avTpcC
 9xRetRqa7jA445c/r0JwaOXSaS84KTXEY9A7X1Me29WyAPZoxStnJajp7z4ZAiezy
10WUtk5J0JTbRu9Hf1DqRKFKMmywnWMREmJ/zuOHHoYDgLQ2yLzfKgcAn3z853hcno
11uWJ2tlY53i7L2YrM9oi8XcxjQ4RpLPB57uHGvc3RKNGFfmtB20+8OFKsbvCwpyQM
12cbl1NeWW/Cog8e8aoSlJGLsdfET90E6xYfGJIlxsRaN3QG7BLRK+Y72+jOgLulkQ
13j3d8/rwloWRW5Iz/nDVMM5zj6DmbwWCiYxWGZk9ygNvY4Jh+8i9d5Wa78Z6p1BHU
14V2RaKA/reYduiEd/ds4oPjpSq3JFCFPopVdOxQGbCx0VlDZKodT1s1yffVE8SqAE
15r37wmoMd
16=mYsj
17-----END PGP SIGNATURE-----

Timestamp of file with hash 55d66b1be7c55577a29116b202deff842019db2cbb0adf69487cb6e652257741 -

MarcoFalke added this to the milestone 0.19.0 on Jul 25, 2019

in src/net_processing.h:94 in 9bc8b28c1d

89@@ -90,4 +90,7 @@ struct CNodeStateStats {
90 /** Get statistics from node state */
91 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
92 
93+/** Relay transaction to every node */
94+void RelayTransaction(const uint256&, const CConnman& connman);

jnewbery commented at 3:23 pm on July 25, 2019:

nit: Please add the name of the txid argument:

void RelayTransaction(const uint256& txid, const CConnman& connman);

in src/node/transaction.cpp:7 in 9bc8b28c1d

4@@ -5,6 +5,7 @@
5 
6 #include <consensus/validation.h>
7 #include <net.h>

jnewbery commented at 3:26 pm on July 25, 2019:

nit: you can remove the net.h include now that CNode is not used in this file.

in src/interfaces/chain.cpp:11 in 9bc8b28c1d

 8@@ -9,6 +9,7 @@
 9 #include <interfaces/handler.h>
10 #include <interfaces/wallet.h>
11 #include <net.h>

jnewbery commented at 3:28 pm on July 25, 2019:

nit: you can remove the net.h include now that this file doesn’t use CNode

jnewbery commented at 3:29 pm on July 25, 2019: member

ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9

Three nits inline. Take ’em or leave ’em.

ariard commented at 5:13 pm on July 25, 2019: member

thanks @jnewbery will correct the nits in one of my other PRs to not invalidate reviews

jonatack commented at 12:50 pm on July 31, 2019: member

ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9, second @jnewbery’s suggestions, my guess is they could be added without risking delaying this PR.

jnewbery commented at 2:23 pm on July 31, 2019: member

I think this is ready for merge. It could go in with or without https://github.com/jnewbery/bitcoin/commit/72f29c34c069d7cd87424f3c4dbefd4aa25400e0 (I’ll open a quick follow-up PR if we merge this as is)

ariard commented at 2:25 pm on July 31, 2019: member

Okay, will rebase #15713 soon after and address nits on it at same time

jonatack commented at 2:30 pm on July 31, 2019: member

NP. FWIW I also built and tested with @jnewbery’s changes and all pass.

 0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 1index 22e4aaedaf..30966c50ad 100644
 2--- a/src/interfaces/chain.cpp
 3+++ b/src/interfaces/chain.cpp
 4@@ -8,7 +8,6 @@
 5 #include <chainparams.h>
 6 #include <interfaces/handler.h>
 7 #include <interfaces/wallet.h>
 8-#include <net.h>
 9 #include <net_processing.h>
10 #include <node/coin.h>
11 #include <policy/fees.h>
12diff --git a/src/net_processing.h b/src/net_processing.h
13index 1d26164b18..c60ed4b21f 100644
14--- a/src/net_processing.h
15+++ b/src/net_processing.h
16@@ -91,6 +91,6 @@ struct CNodeStateStats {
17 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
18 
19 /** Relay transaction to every node */
20-void RelayTransaction(const uint256&, const CConnman& connman);
21+void RelayTransaction(const uint256& txid, const CConnman& connman);
22 
23 #endif // BITCOIN_NET_PROCESSING_H
24diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
25index 5cd567a5c4..ae3edd5d6a 100644
26--- a/src/node/transaction.cpp
27+++ b/src/node/transaction.cpp
28@@ -4,7 +4,6 @@
29 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
30 
31 #include <consensus/validation.h>
32-#include <net.h>
33 #include <net_processing.h>
34 #include <txmempool.h>
35 #include <util/validation.h>

MarcoFalke referenced this in commit 7821821a23 on Jul 31, 2019

MarcoFalke merged this on Jul 31, 2019

MarcoFalke closed this on Jul 31, 2019

MarcoFalke commented at 2:55 pm on July 31, 2019: member

net.h might still be included through net_processing.h, so the follow-up changes are only stylistic

sidhujag referenced this in commit eb1cc54b8a on Aug 1, 2019

deadalnix referenced this in commit ac6a10c8b6 on May 28, 2020

kittywhiskers referenced this in commit 1448c56ea0 on Dec 4, 2021

kittywhiskers referenced this in commit 80fdadd83f on Dec 8, 2021

kittywhiskers referenced this in commit 2a2e02dc9c on Dec 8, 2021

kittywhiskers referenced this in commit 6cbf0793af on Dec 12, 2021

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

kittywhiskers referenced this in commit b905323838 on Dec 12, 2021

kittywhiskers referenced this in commit 334716741b on Dec 13, 2021

kittywhiskers referenced this in commit 9340d044e5 on Dec 13, 2021

kittywhiskers referenced this in commit ce13bb4724 on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021

refactor: use RelayTransaction in BroadcastTransaction utility #16452

Conflicts