Skip to content

refactor: Use Txid in CMerkleBlock #28958

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
Merged
fanquake merged 1 commit into from
Nov 29, 2023
Merged

refactor: Use Txid in CMerkleBlock #28958

fanquake merged 1 commit into from
Nov 29, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 28, 2023

This should also fix a gcc-13 compiler warning, see #28922 (comment)

rpc/txoutproof.cpp: In lambda function:
rpc/txoutproof.cpp:72:33: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
   72 |                     const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
      |                                 ^~~~
rpc/txoutproof.cpp:72:52: note: the temporary was destroyed at the end of the full expression ‘AccessByTxid((*(const CCoinsViewCache*)(&(& active_chainstate)->Chainstate::CoinsTip())), transaction_identifier<false>::FromUint256((* & tx)))’
   72 |                     const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
      |                                        ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 28, 2023 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@maflcko maflcko force-pushed the 2311-ref-txid- branch 3 times, most recently from fa2c5d4 to fa66571 Compare November 28, 2023 16:05
@maflcko maflcko mentioned this pull request Nov 28, 2023
Merged
@dergoegge
Copy link
Member

dergoegge commented Nov 28, 2023

Concept ACK

dergoegge
dergoegge approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge 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 fa665711a00b2e39954848160b3e70c60730b276

sedited
sedited approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK fa665711a00b2e39954848160b3e70c60730b276

@sedited
Copy link
Contributor

sedited commented Nov 29, 2023

Re-ACK fa02c08

@DrahtBot DrahtBot requested a review from dergoegge November 29, 2023 07:33
dergoegge
dergoegge approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

reACK fa02c08

@fanquake fanquake merged commit 8cf2137 into bitcoin:master Nov 29, 2023
@fanquake
Copy link
Member

fanquake commented Nov 29, 2023

cc @achow101

@maflcko maflcko deleted the 2311-ref-txid- branch November 29, 2023 11:01
@achow101
Copy link
Member

achow101 commented Nov 29, 2023

Post merge ACK fa02c08

@bitcoin bitcoin locked and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

+1 more reviewer

@dergoegge dergoegge dergoegge approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @dergoegge @sedited @fanquake @achow101