refactor: move GetTransaction to node/transaction.cpp #22528

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202107-refactor-move_GetTransaction changing 6 files +61 −50

theStack commented at 1:19 PM on July 22, 2021: member

~This PR is based on #22383, which should be reviewed first~ (merged by now).

In yesterday's PR review club session to PR 22383, the idea of moving the function GetTransaction(...) from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in lint-circular-dependencies.sh). Thanks to jnewbery for suggesting and to sipa for providing historical background.

Relevant IRC log:

17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now

The commit should be trivial to review with --color-moved.

theStack force-pushed on Jul 22, 2021
in src/node/transaction.h:9 in 8cc5beb16c outdated
```
   5 | @@ -6,10 +6,13 @@
   6 |  #define BITCOIN_NODE_TRANSACTION_H
   7 |  
   8 |  #include <attributes.h>
   9 | +#include <consensus/params.h>
```
jnewbery commented at 1:31 PM on July 22, 2021:

You don't need to include this. You can forward declare Consensus::Params:

namespace Consensus { struct Params; }
theStack commented at 1:54 PM on July 22, 2021:

Ah, thanks! I was naively only greping for Consensus::Params; to look how other headers would forward-declare this (concluding that they don't and including the header is the preferred way), which is obviously not how it works :grimacing:
jnewbery commented at 1:31 PM on July 22, 2021: member

Concept ACK. I have a branch that does exactly the same thing :)
refactor: move `GetTransaction(...)` to node/transaction.cpp
can be reviewed with --color-moved
abc57e1f08
theStack force-pushed on Jul 22, 2021
theStack commented at 1:58 PM on July 22, 2021: member

Concept ACK. I have a branch that does exactly the same thing :)

My bad, should have communicated before to avoid double work. Feel free to open yours and I'll happily review :)
jnewbery commented at 1:59 PM on July 22, 2021: member

My bad, should have communicated before to avoid double work. Feel free to open yours and I'll happily review :)

Not a problem at all! It's good to see that we separately arrived at the same answer :)
DrahtBot added the label Refactoring on Jul 22, 2021
DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2021
DrahtBot added the label Validation on Jul 22, 2021
LarryRuane commented at 5:04 PM on July 22, 2021: contributor

concept ACK

in test/lint/lint-circular-dependencies.sh:13 in abc57e1f08 outdated

   9 | @@ -10,7 +10,6 @@ export LC_ALL=C
  10 |  
  11 |  EXPECTED_CIRCULAR_DEPENDENCIES=(
  12 |      "chainparamsbase -> util/system -> chainparamsbase"
  13 | -    "index/txindex -> validation -> index/txindex"

promag commented at 5:16 PM on July 22, 2021:

🎉

promag commented at 6:15 PM on July 22, 2021: member

Code review ACK abc57e1f0882a1a2bb20474648419979af6e383d. With git show --color-moved=dimmed_zebra verified the move-only, #includes changes, added forward declarations and drop of circular dependency 🎉
promag commented at 6:17 PM on July 22, 2021: member

Happy to re-ACK with #22383 (review)
doc: GetTransaction()/getrawtransaction follow-ups to #22383 f685a13bef
theStack commented at 6:41 PM on July 22, 2021: member

Added a commit with jnewbery's comment/documentation follow-ups from #22383, as suggested by MarcoFalke. Was not sure if the RPC doc change is also appropriate to put in this PR (since it was only concerned with the GetTransaction function in the beginning), OTOH it is called by the getrawtransaction RPC, so it should be okay.
DrahtBot commented at 9:40 PM on July 22, 2021: 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:
- #21726 by fjahr
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.
jnewbery commented at 9:16 AM on July 23, 2021: member

Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec

Thanks for doing this!
rajarshimaitra commented at 2:07 PM on July 23, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22528/commits/f685a13bef0418663015ea6d8f448f075510c0ec

Had put it in my potential todos. @theStack at the work amazingly fast.. 🔥
in src/node/transaction.cpp:113 in f685a13bef
```
 106 | @@ -104,3 +107,38 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
 107 |  
 108 |      return TransactionError::OK;
 109 |  }
 110 | +
 111 | +CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock)
 112 | +{
 113 | +    LOCK(cs_main);
```
rajarshimaitra commented at 5:01 AM on July 24, 2021:

Maybe it's also a good opportunity to address #22383 (review) here?

mjdietzx commented at 5:47 PM on July 24, 2021:

+1

jnewbery commented at 6:13 PM on July 24, 2021:

I'd say no. Better to keep this as move only and leave lock cleanups for a future PR. That makes this easier to review (and avoids invalidating the three ACKs on the current branch).

theStack commented at 9:13 AM on July 25, 2021:

The PR was planned to be one of the move-only refactoring kinds that should be trivial to review (strictly speaking, with the follow-up commit changing the RPC help text it's not a pure refactor anymore, but that I'd still consider acceptable). Changing locks is significantly harder to review, so I tend to agree with jnewbery here and leave it as it is by now.
mjdietzx commented at 5:47 PM on July 24, 2021: contributor

crACK f685a13bef0418663015ea6d8f448f075510c0ec
practicalswift commented at 7:53 PM on July 24, 2021: contributor

Concept ACK

Very nice to see the list of circular dependencies shrink :)
LarryRuane commented at 8:04 PM on July 24, 2021: contributor

Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
theStack force-pushed on Jul 28, 2021
theStack commented at 3:57 PM on July 28, 2021: member

Rebased on master (now that #22383 is merged, there is no need to include its commit anymore).
MarcoFalke commented at 4:00 PM on July 28, 2021: member

Please don't force push, as this requires re-review. This is a GitHub bug that doesn't affect us.
theStack force-pushed on Jul 28, 2021
theStack commented at 4:10 PM on July 28, 2021: member

Please don't force push, as this requires re-review. This is a GitHub bug that doesn't affect us.

Ah, sorry wasn't aware that displaying the first commit is happening due to a GitHub bug (if I understood you correctly). Force-pushed back to how it was before.
MarcoFalke merged this on Jul 28, 2021
MarcoFalke closed this on Jul 28, 2021
promag commented at 6:56 PM on July 28, 2021: member

ACK f685a13bef0418663015ea6d8f448f075510c0ec. Only changes since last review are new comment about reorg and bip30 and improved help text.
sidhujag referenced this in commit 013aa39a9b on Jul 29, 2021
theStack deleted the branch on Jul 31, 2021
DrahtBot locked this on Aug 16, 2022