Skip to content

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

Merged
maflcko merged 2 commits intobitcoin:masterfrom
theStack:202107-refactor-move_GetTransaction
Jul 28, 2021
Merged

refactor: move GetTransaction to node/transaction.cpp#22528
maflcko merged 2 commits intobitcoin:masterfrom
theStack:202107-refactor-move_GetTransaction

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Jul 22, 2021 •
edited
Loading

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 theStack force-pushed the 202107-refactor-move_GetTransaction branch from 52d93cb to 8cc5beb Compare July 22, 2021 13:26
jnewbery
jnewbery reviewed Jul 22, 2021
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@theStack theStack force-pushed the 202107-refactor-move_GetTransaction branch from 8cc5beb to abc57e1 Compare July 22, 2021 13:54
@theStack
Copy link
Contributor Author

theStack commented Jul 22, 2021

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
Copy link
Contributor

jnewbery commented Jul 22, 2021

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 :)

@LarryRuane
Copy link
Contributor

LarryRuane commented Jul 22, 2021

concept ACK

promag
promag reviewed Jul 22, 2021
View reviewed changes
test/lint/lint-circular-dependencies.sh

EXPECTED_CIRCULAR_DEPENDENCIES=(
"chainparamsbase -> util/system -> chainparamsbase"
"index/txindex -> validation -> index/txindex"
Copy link
Contributor

@promag promag Jul 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

promag
promag reviewed Jul 22, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK abc57e1. With git show --color-moved=dimmed_zebra verified the move-only, #includes changes, added forward declarations and drop of circular dependency 🎉

@maflcko maflcko mentioned this pull request Jul 22, 2021
Merged
@promag
Copy link
Contributor

promag commented Jul 22, 2021

Happy to re-ACK with #22383 (comment)

@theStack
Copy link
Contributor Author

theStack commented Jul 22, 2021

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
Copy link
Contributor

DrahtBot commented Jul 22, 2021

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

Conflicts

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.

@DrahtBot DrahtBot mentioned this pull request Jul 23, 2021
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Jul 23, 2021

Code review ACK f685a13

Thanks for doing this!

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Jul 23, 2021

tACK f685a13

Had put it in my potential todos. @theStack at the work amazingly fast.. 🔥

rajarshimaitra
rajarshimaitra reviewed Jul 24, 2021
View reviewed changes
@mjdietzx
Copy link
Contributor

mjdietzx commented Jul 24, 2021

crACK f685a13

@practicalswift
Copy link
Contributor

practicalswift commented Jul 24, 2021

Concept ACK

Very nice to see the list of circular dependencies shrink :)

@LarryRuane
Copy link
Contributor

LarryRuane commented Jul 24, 2021

Code review, test ACK f685a13

@theStack theStack force-pushed the 202107-refactor-move_GetTransaction branch from f685a13 to d300c2c Compare July 28, 2021 15:55
@theStack
Copy link
Contributor Author

theStack commented Jul 28, 2021

Rebased on master (now that #22383 is merged, there is no need to include its commit anymore).

@maflcko
Copy link
Member

maflcko commented Jul 28, 2021

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

@theStack theStack force-pushed the 202107-refactor-move_GetTransaction branch from d300c2c to f685a13 Compare July 28, 2021 16:09
@theStack
Copy link
Contributor Author

theStack commented Jul 28, 2021

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.

@maflcko maflcko merged commit 4b1fb50 into bitcoin:master Jul 28, 2021
promag
promag reviewed Jul 28, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f685a13. Only changes since last review are new comment about reorg and bip30 and improved help text.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 29, 2021
Merge bitcoin#22528: refactor: move GetTransaction to node/transactio…
013aa39 
…n.cpp
@theStack theStack deleted the 202107-refactor-move_GetTransaction branch July 31, 2021 20:03
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@jnewbery jnewbery jnewbery left review comments

@promag promag promag left review comments

@mjdietzx mjdietzx mjdietzx left review comments

@rajarshimaitra rajarshimaitra rajarshimaitra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@theStack @jnewbery @LarryRuane @promag @DrahtBot @rajarshimaitra @mjdietzx @practicalswift @maflcko