Skip to content

test: XORed blocks test follow up #30669

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
achow101 merged 6 commits into from
Aug 26, 2024
Merged

test: XORed blocks test follow up #30669

achow101 merged 6 commits into from
Aug 26, 2024

Conversation

@tdb3
Copy link
Contributor

@tdb3 tdb3 commented Aug 17, 2024 •
edited
Loading

Builds on PR #30657.

Refactors read_xor_key() from util.py to test_node.py (comment #30657 (comment))

Adds a check that xor.dat is created when missing (comment #30657 (comment))

Help states:

-blocksxor
       Whether an XOR-key applies to blocksdir *.dat files. The created XOR-key
       will be zeros for an existing blocksdir or when `-blocksxor=0` is
       set, and random for a freshly initialized blocksdir. (default: 1)

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 17, 2024 •
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 maflcko, theStack, brunoerg, achow101
Stale ACK danielabrozzoni, glozow

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

@DrahtBot DrahtBot added the Tests label Aug 17, 2024
danielabrozzoni
danielabrozzoni approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

tACK 07365d026127eb8435e7919a7793bd25d7f3ceca

brunoerg
brunoerg reviewed Aug 19, 2024
View reviewed changes
@tdb3
Copy link
Contributor Author

tdb3 commented Aug 19, 2024

Thanks @danielabrozzoni and @brunoerg. Pushed an update to address comments, and clarified the log message.

theStack
theStack approved these changes Aug 20, 2024
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.

ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

Thanks for following up! If you want, you could also tackle the suggestion to move the read_xor_key helper to the TestNode class, see #30657 (comment)

@theStack theStack mentioned this pull request Aug 20, 2024
Merged
glozow
glozow reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

brunoerg
brunoerg approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

@tdb3
Copy link
Contributor Author

tdb3 commented Aug 21, 2024

If you want, you could also tackle the suggestion to move the read_xor_key helper to the TestNode class, see #30657 (comment)

Thanks for the suggestion! This PR seems to be a reasonable place to perform the move, so I updated it to include this. Happy to make tweaks if you or @maflcko have any further suggestions.

@tdb3 tdb3 changed the title test: check xor.dat creation when missing test: XORed blocks test follow up Aug 21, 2024
theStack
theStack reviewed Aug 22, 2024
View reviewed changes
@tdb3
test: add blocks_key_path
c8176f7 
Adds a convenience function to TestNode
to provide the path to the blocks xor key.
Updates util and feature_blocksxor to use it.
theStack
theStack approved these changes Aug 24, 2024
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.

ACK ab9d2451c47fd30abecaea3f8d8a6e33541911d7

@DrahtBot DrahtBot requested review from brunoerg and glozow August 24, 2024 23:22
theStack
theStack approved these changes Aug 24, 2024
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.

re-ACK 1c403af92be13cbdfa6ab1906f8cc34fabc21e63

maflcko
maflcko reviewed Aug 25, 2024
View reviewed changes
@maflcko
Copy link
Member

maflcko commented Aug 26, 2024

ACK e1d5dd7

@DrahtBot DrahtBot requested a review from theStack August 26, 2024 07:32
theStack
theStack approved these changes Aug 26, 2024
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.

re-ACK e1d5dd7

brunoerg
brunoerg approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

reACK e1d5dd7

@maflcko maflcko added this to the 28.0 milestone Aug 26, 2024
@achow101
Copy link
Member

achow101 commented Aug 26, 2024

ACK e1d5dd7

@achow101 achow101 merged commit d50f0ce into bitcoin:master Aug 26, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Aug 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

@glozow glozow Awaiting requested review from glozow

3 more reviewers

@maflcko maflcko maflcko left review comments

@theStack theStack theStack approved these changes

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

28.0

Development

Successfully merging this pull request may close these issues.

8 participants

@tdb3 @DrahtBot @maflcko @achow101 @theStack @brunoerg @danielabrozzoni @glozow