Skip to content

refactor: CheckFinalTx pass by reference instead of pointer #22282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

refactor: CheckFinalTx pass by reference instead of pointer #22282

wants to merge 1 commit into from

Conversation

klementtan
Copy link
Contributor

@klementtan klementtan commented Jun 19, 2021 •
edited
Loading

Rationale This PR fixes TODO from #21055

bitcoin/src/validation.cpp

Line 183 in 965e937

assert(active_chain_tip); // TODO: Make active_chain_tip a reference

#20750 (comment) for additional information.

Changes to CheckFinalTx: active_chain_tip to be passed by reference instead of pointer and shift & of tx to be left-attaching

@klementtan klementtan changed the title refactor: CheckFinalTx pass by reference instead of ptr refactor: CheckFinalTx pass by reference instead of pointer Jun 19, 2021
theStack
theStack approved these changes Jun 19, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack 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 10caa3e 🥥

benthecarman
benthecarman reviewed Jun 20, 2021
View reviewed changes
Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

ack 10caa3e

@practicalswift
Copy link
Contributor

Concept ACK

Candidates for similar treatment can be found using the git grep commands in #19062 :)

@klementtan klementtan mentioned this pull request Jun 20, 2021
Closed
@klementtan
Copy link
Contributor Author

Candidates for similar treatment can be found using the git grep commands in #19062 :)

Thanks for informing me be about this. Added a comment on the issue to reference this PR. However, I think I will keep the scope of this PR to CheckFianlTx to make it easy to review.

@DrahtBot DrahtBot mentioned this pull request Aug 10, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 10, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22677 (cut the validation <-> txmempool circular dependency 2/2 by glozow)

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.

maflcko
maflcko reviewed Dec 1, 2021
View reviewed changes
src/validation.cpp
{
AssertLockHeld(cs_main);
assert(active_chain_tip); // TODO: Make active_chain_tip a reference
Copy link
Member

Choose a reason for hiding this comment

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

This removes an assertion and turns a (theoretical) crash into UB

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 1, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@klementtan
Copy link
Contributor Author

Closing this PR as this refactor will remove an assertion.

@klementtan klementtan closed this Dec 12, 2021
@maflcko
Copy link
Member

maflcko commented Dec 13, 2021

As long as you preserve the assertion, it should be ok.

@bitcoin bitcoin locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@maflcko maflcko maflcko left review comments

@benthecarman benthecarman benthecarman left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@klementtan @practicalswift @DrahtBot @maflcko @theStack @benthecarman