Skip to content

addrman: Log too low compat value #24312

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
merged 1 commit into from
Mar 8, 2022
Merged

addrman: Log too low compat value #24312

merged 1 commit into from
Mar 8, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 10, 2022 •
edited
Loading

Before this patch, when writing a negative lowest_compatible value, it would be read as a positive value. For example -32 will be read as 224. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.

In practice none of this should ever happen. Bitcoin Core would never write a negative lowest_compatible in normal operation, unless the file storage is later corrupted by external influence.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 11, 2022 •
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:

  • #23962 (Use int32_t type for transaction size/weight consistently by hebasto)

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.

shaavan
shaavan reviewed Feb 11, 2022
View reviewed changes
vasild
vasild reviewed Feb 11, 2022
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Feb 11, 2022
Merged
@maflcko maflcko force-pushed the 2202-logNeg branch 2 times, most recently from fa943a0 to fac3277 Compare February 11, 2022 18:31
@maflcko maflcko changed the title addrman: Log negative lowest_compatible addrman: Log too low compat value Feb 11, 2022
ghost
ghost approved these changes Feb 12, 2022
View reviewed changes
Copy link

@ghost ghost 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 fac3277

I wish we had a hidden RPC for:

write_addrman(peers_dat, lowest_compatible=-32)

@DrahtBot DrahtBot mentioned this pull request Feb 12, 2022
Merged
@maflcko
Copy link
Member Author

maflcko commented Feb 12, 2022 •
edited
Loading

I wish we had a hidden RPC for:

I wish we don't. Generally I am not a fan of baking test code into the production system, unless there is a need for it.

shaavan
shaavan approved these changes Feb 12, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK fac3277

Changes since my last review:

  • Instead of changing the type of lowest_compatible from unsigned to signed integer, a condition statement has been added, which removes the need for signed lowest_compatible.

I think the wording of added failure message is correct and adequate. I experimented with several different values of the lowest_compatible argument in the test, and each time the error message remained logical and consistent, with changing values of compat.

Note:
The test failed with lowest_compatible=-33, as this would make compat=-1, which is out of range, but I don't think this condition would ever arise in real-life situations.

This was referenced Feb 13, 2022
Merged
Merged
vasild
vasild reviewed Feb 14, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

Is the first commit test: Remove no longer needed suppressions related to the low compat issue? The commit message does not explain why the suppressions are no longer needed.

@vasild
Copy link
Contributor

vasild commented Feb 14, 2022

The test failed with lowest_compatible=-33, as this would make compat=-1, which is out of range, but I don't think this condition would ever arise in real-life situations.

The test fails with struct.error: ubyte format requires 0 <= number <= 255 because the python test code refuses to write -1 with the B format (see https://docs.python.org/3/library/struct.html) to disk. The addrman deserialization is not even invoked in this case.

@maflcko
Copy link
Member Author

maflcko commented Feb 14, 2022

A compat value of -1 doesn't exist. compat is unsigned in both Bitcoin Core and the python mock.

vasild
vasild reviewed Feb 15, 2022
View reviewed changes
vasild
vasild approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK faaa6c2

What about #24312 (review)?

@fanquake fanquake requested a review from mzumsande February 23, 2022 16:21
mzumsande
mzumsande reviewed Feb 23, 2022
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande 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 faaa6c2 - also did some light testing.

@fanquake fanquake requested review from vasild and mzumsande February 24, 2022 14:13
@mzumsande
Copy link
Contributor

ACK fa0fb76

Only change: Rewording of the error message

addrman: Log too low compat value
fa097d0 
Also remove uint8_t{} casts from values that are already of the same
type.
@mzumsande
Copy link
Contributor

re-ACK fa097d0

vasild
vasild approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fa097d0

Even better wording, thanks @mzumsande!

@bitcoin bitcoin deleted a comment Mar 8, 2022
@maflcko maflcko merged commit b07fdd7 into bitcoin:master Mar 8, 2022
@maflcko maflcko deleted the 2202-logNeg branch March 8, 2022 15:50
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 8, 2022
Merge bitcoin#24312: addrman: Log too low compat value
65d7e58
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 21, 2022
@Fabcien
addrman: Log too low compat value
afbc199 
Summary:
```
Also remove uint8_t{} casts from values that are already of the same type.
```

Backport of [[bitcoin/bitcoin#24312 | core#24312]].

Depends on D12348.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D12349
@bitcoin bitcoin locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mzumsande mzumsande Awaiting requested review from mzumsande

3 more reviewers

@vasild vasild vasild approved these changes

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @vasild @mzumsande @shaavan