addrman: Log too low compat value by maflcko · Pull Request #24312 · bitcoin/bitcoin

maflcko · 2022-02-10T16:12:56Z

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 · 2022-02-11T11:00:45Z

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.

src/addrman.cpp

ghost

Code Review ACK fac3277

I wish we had a hidden RPC for:

write_addrman(peers_dat, lowest_compatible=-32)

maflcko · 2022-02-12T07:18:21Z

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

ACK fac32775c8e451527b48e855aebd13b7ed0e820d

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.

vasild

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.

src/addrman.cpp

vasild · 2022-02-14T11:36:58Z

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 · 2022-02-14T12:14:26Z

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

src/addrman.cpp

vasild

ACK faaa6c2af56eedaf8d50b33015d98c7bc289c2f4

What about #24312 (review)?

mzumsande

Code Review ACK faaa6c2af56eedaf8d50b33015d98c7bc289c2f4 - also did some light testing.

src/addrman.cpp

mzumsande · 2022-02-24T15:12:30Z

ACK fa0fb76b7772eda83ec9c7030911a63331f590c5

Only change: Rewording of the error message

Also remove uint8_t{} casts from values that are already of the same type.

mzumsande · 2022-03-02T13:21:27Z

re-ACK fa097d0

vasild

ACK fa097d0

Even better wording, thanks @mzumsande!

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

maflcko added Refactoring Utils/log/libs labels Feb 10, 2022

shaavan reviewed Feb 11, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

src/addrman.cpp Outdated Show resolved Hide resolved

vasild reviewed Feb 11, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 11, 2022

p2p: Avoid InitError when downgrading peers.dat #24201

Merged

maflcko force-pushed the 2202-logNeg branch 2 times, most recently from fa943a0 to fac3277 Compare February 11, 2022 18:31

maflcko changed the title ~~addrman: Log negative lowest_compatible~~ addrman: Log too low compat value Feb 11, 2022

ghost approved these changes Feb 12, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 12, 2022

Use int32_t type for most transaction size/weight values #23962

Merged

shaavan approved these changes Feb 12, 2022

View reviewed changes

This was referenced Feb 13, 2022

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

Fix unsigned integer overflows in interpreter #24214

Merged

vasild reviewed Feb 14, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2202-logNeg branch from fac3277 to fab4df1 Compare February 14, 2022 16:36

vasild reviewed Feb 15, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2202-logNeg branch from fab4df1 to faaa6c2 Compare February 15, 2022 08:51

vasild approved these changes Feb 15, 2022

View reviewed changes

fanquake requested a review from mzumsande February 23, 2022 16:21

mzumsande reviewed Feb 23, 2022

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2202-logNeg branch from faaa6c2 to fa0fb76 Compare February 24, 2022 13:45

fanquake requested review from mzumsande and vasild February 24, 2022 14:13

DrahtBot added the Needs rebase label Feb 25, 2022

maflcko force-pushed the 2202-logNeg branch from fa0fb76 to fa92e1b Compare February 25, 2022 08:41

DrahtBot removed the Needs rebase label Feb 25, 2022

addrman: Log too low compat value

fa097d0

Also remove uint8_t{} casts from values that are already of the same type.

maflcko force-pushed the 2202-logNeg branch from fa92e1b to fa097d0 Compare February 25, 2022 13:27

vasild approved these changes Mar 8, 2022

View reviewed changes

bitcoin deleted a comment Mar 8, 2022

maflcko merged commit b07fdd7 into bitcoin:master Mar 8, 2022

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

bitcoin locked and limited conversation to collaborators Mar 8, 2023

Conversation

maflcko commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Feb 14, 2022

Uh oh!

maflcko commented Feb 14, 2022

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Feb 24, 2022

Uh oh!

mzumsande commented Mar 2, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maflcko commented Feb 10, 2022 •

edited

Loading

DrahtBot commented Feb 11, 2022 •

edited

Loading

maflcko commented Feb 12, 2022 •

edited

Loading