refactor: use RelayTransaction in BroadcastTransaction utility #16452

pull ariard wants to merge 1 commits into bitcoin:master from ariard:2019-07-reuse-relay-tx changing 4 files +13 −12
  1. 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.

  2. DrahtBot commented at 5:59 PM on July 24, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  3. MarcoFalke renamed this:
    refactor : use RelayTransaction in BroadcastTransaction utility
    refactor: use RelayTransaction in BroadcastTransaction utility
    on Jul 24, 2019
  4. MarcoFalke commented at 6:54 PM on July 24, 2019: member

    re-run ci

  5. MarcoFalke closed this on Jul 24, 2019
  6. MarcoFalke reopened this on Jul 24, 2019
  7. 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.

  8. 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.

    void 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?

  9. MarcoFalke approved
  10. MarcoFalke commented at 7:08 PM on July 24, 2019: member

    ACK ddab0452020bba2dcdd342437454cd01227d401e

  11. DrahtBot added the label P2P on Jul 24, 2019
  12. DrahtBot added the label Refactoring on Jul 24, 2019
  13. promag commented at 7:41 PM on July 24, 2019: member

    ACK, also Marco suggestions.

  14. ariard force-pushed on Jul 24, 2019
  15. 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

  16. ariard force-pushed on Jul 24, 2019
  17. 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: 
         CInv inv(MSG_TX, txid);
  18. 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

  19. 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: 
    void RelayTransaction(const uint256& txid, const CConnman& connman);
  20. MarcoFalke commented at 9:16 PM on July 24, 2019: member

    re-ACK apart from compile failures

  21. ariard force-pushed on Jul 24, 2019
  22. ariard commented at 9:48 PM on July 24, 2019: member

    Sorry for compilation failures, should be good now.

  23. 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

        RelayTransaction(hashTx, *g_connman);

    Also, what happened to this suggestion:

    diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 02f39cef8e..22e4aaedaf 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -9,6 +9,7 @@
 #include <interfaces/handler.h>
 #include <interfaces/wallet.h>
 #include <net.h>
+#include <net_processing.h>
 #include <node/coin.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
@@ -292,8 +293,7 @@ public:
     }
     void relayTransaction(const uint256& txid) override
     {
-        CInv inv(MSG_TX, txid);
-        g_connman->ForEachNode([&inv](CNode* node) { node->PushInventory(inv); });
+        RelayTransaction(txid, *g_connman);
     }
     void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override
     {
    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

  24. ariard force-pushed on Jul 24, 2019
  25. refactor : use RelayTransaction in BroadcastTransaction utility
    To do so, we also refactor RelayTransaction to take a txid
instead of passing a tx
    9bc8b28c1d
  26. ariard force-pushed on Jul 24, 2019
  27. 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 12:33 AM on July 25, 2019:

    µnit, pardon

    [&inv](CNode* pnode) {
    MarcoFalke commented at 11:48 AM on July 25, 2019:

    I'd rather keep it the way it is now

  28. promag commented at 12:37 AM on July 25, 2019: member

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

  29. MarcoFalke commented at 11:55 AM on July 25, 2019: member

    ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjNYgv/ecqESlAxc75rOwly8VCGKZjH4Zdb0+lfb3LnFTNgsPkd2OT1UZ9t3qby
J18SGBradFqTrRIivGqwxnOkIKqc2HO+7g2M2NERzdudVE0TOauFIMC+R0avTpcC
xRetRqa7jA445c/r0JwaOXSaS84KTXEY9A7X1Me29WyAPZoxStnJajp7z4ZAiezy
WUtk5J0JTbRu9Hf1DqRKFKMmywnWMREmJ/zuOHHoYDgLQ2yLzfKgcAn3z853hcno
uWJ2tlY53i7L2YrM9oi8XcxjQ4RpLPB57uHGvc3RKNGFfmtB20+8OFKsbvCwpyQM
cbl1NeWW/Cog8e8aoSlJGLsdfET90E6xYfGJIlxsRaN3QG7BLRK+Y72+jOgLulkQ
j3d8/rwloWRW5Iz/nDVMM5zj6DmbwWCiYxWGZk9ygNvY4Jh+8i9d5Wa78Z6p1BHU
V2RaKA/reYduiEd/ds4oPjpSq3JFCFPopVdOxQGbCx0VlDZKodT1s1yffVE8SqAE
r37wmoMd
=mYsj
-----END PGP SIGNATURE-----

    Timestamp of file with hash 55d66b1be7c55577a29116b202deff842019db2cbb0adf69487cb6e652257741 -

    </details>

  30. MarcoFalke added this to the milestone 0.19.0 on Jul 25, 2019
  31. 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);

  32. 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.

  33. 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

  34. jnewbery commented at 3:29 PM on July 25, 2019: member

    ACK 9bc8b28c1d26c28edf4bbc890be97c0ad7a73cb9

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

  35. 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

  36. 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.

  37. 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)

  38. 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

  39. 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.

    diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 22e4aaedaf..30966c50ad 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -8,7 +8,6 @@
 #include <chainparams.h>
 #include <interfaces/handler.h>
 #include <interfaces/wallet.h>
-#include <net.h>
 #include <net_processing.h>
 #include <node/coin.h>
 #include <policy/fees.h>
diff --git a/src/net_processing.h b/src/net_processing.h
index 1d26164b18..c60ed4b21f 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -91,6 +91,6 @@ struct CNodeStateStats {
 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
 
 /** Relay transaction to every node */
-void RelayTransaction(const uint256&, const CConnman& connman);
+void RelayTransaction(const uint256& txid, const CConnman& connman);
 
 #endif // BITCOIN_NET_PROCESSING_H
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 5cd567a5c4..ae3edd5d6a 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -4,7 +4,6 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <consensus/validation.h>
-#include <net.h>
 #include <net_processing.h>
 #include <txmempool.h>
 #include <util/validation.h>
  40. MarcoFalke referenced this in commit 7821821a23 on Jul 31, 2019
  41. MarcoFalke merged this on Jul 31, 2019
  42. MarcoFalke closed this on Jul 31, 2019
  43. 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

  44. sidhujag referenced this in commit eb1cc54b8a on Aug 1, 2019
  45. deadalnix referenced this in commit ac6a10c8b6 on May 28, 2020
  46. kittywhiskers referenced this in commit 1448c56ea0 on Dec 4, 2021
  47. kittywhiskers referenced this in commit 80fdadd83f on Dec 8, 2021
  48. kittywhiskers referenced this in commit 2a2e02dc9c on Dec 8, 2021
  49. kittywhiskers referenced this in commit 6cbf0793af on Dec 12, 2021
  50. kittywhiskers referenced this in commit 4cf093b353 on Dec 12, 2021
  51. kittywhiskers referenced this in commit b905323838 on Dec 12, 2021
  52. kittywhiskers referenced this in commit 334716741b on Dec 13, 2021
  53. kittywhiskers referenced this in commit 9340d044e5 on Dec 13, 2021
  54. kittywhiskers referenced this in commit ce13bb4724 on Dec 13, 2021
  55. DrahtBot locked this on Dec 16, 2021
Contributors
ariardDrahtBotMarcoFalkejnewberypromagjonatack
Labels
RefactoringP2P

Milestone
0.19.0

Linked (view graph)
#15713 refactor: Replace chain relayTransactions/submitMemoryPool by higher method
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me