p2p: Avoid InitError when downgrading peers.dat by junderw · Pull Request #24201 · bitcoin/bitcoin

junderw · 2022-01-29T13:04:55Z

fixes #24188 (also see #22762 (comment))
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded Bitcoin Core version would cause an InitError.

This commit changes this behavior to overwrite the existing peers.dat with
a new empty one.

junderw · 2022-01-29T13:42:16Z

Part of the reason why I am using strncmp is because AddrMan (AddrManImpl) -> Unserialize might be called somewhere else, so I didn't want to touch it.

I should probably re-work AddrManImpl to use a new exception class that wraps around std::ios_base::failure and only throw exception in the case of version mismatch... but I wanted to get feedback first.

maflcko · 2022-01-29T13:51:01Z

Yeah, I think it should be thrown directly where the version is read

junderw · 2022-01-29T23:36:08Z

Fixed.

Also, 2 jobs on the first commit's CI have been running for 10 hours... is this caused by my strncmp?

junderw · 2022-01-30T23:18:22Z

On second look, I wonder if we should also LogPrintf the e.what() right before we LogPrintf "Creating new peers.dat..."?

kallewoof

ACK 96c9d9c7efa13128df74cebf8dda7d1e5db1f9c6

luke-jr

IMO should be squashed

junderw · 2022-02-06T10:45:57Z

Squashed.

luke-jr

utACK, w/ some thoughts for improvement (can be followups)

src/addrdb.cpp

test/functional/feature_addrman.py

DrahtBot · 2022-02-11T17:21:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24312 (addrman: Log too low compat value by MarcoFalke)

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/addrdb.cpp

maflcko · 2022-02-21T13:25:49Z

Assigned 23.0

michaelfolkson · 2022-02-24T19:24:57Z

@junderw: Can you explain why you made this WIP? One of multiple considerations for whether it should be included in the 23.0 milestone or not.

junderw · 2022-02-24T22:03:43Z

Sorry, I misunderstood. I made it WIP because I thought I still hadn't finished all the outstanding issues with the PR.

After reviewing my own commit I understand I was just confused.

There's a spelling error I'll fix right now.

ghost

tACK 772c9ba

nit:

Not sure about squashing of some commits or the order and type
Suggestion by @luke-jr still not addressed to change peer to peers

junderw · 2022-02-24T23:43:05Z

Unrelated test failed for some reason.

ghost · 2022-02-24T23:52:33Z

Unrelated test failed for some reason.

I don't think that's an issue

Maybe you can follow 1 or 2 or both:

Squash commits according to docs

Have 2 commits:

1st commit: net (src/addrman.h , src/addrman.cpp and src/addrdb.cpp)

2nd commit: test (test/functional/feature_addrman.py)

fixes bitcoin#24188 When downgrading, a peers.dat with a future version that has a minimum required version larger than the downgraded version would cause an InitError. This commit changes this behavior to overwrite the existing peers.dat with a new empty one, while creating a backup in peers.dat.bak.

junderw · 2022-02-25T00:55:23Z

Squashed all commits into one.

ghost

reACK d41ed32

kallewoof

reACK d41ed32

michaelfolkson · 2022-02-25T11:06:14Z

Sorry for the confusion @junderw :) Great to see this has been merged. Thanks!

d41ed32 p2p: Avoid InitError when downgrading peers.dat (junderw) Pull request description: fixes bitcoin#24188 (also see bitcoin#22762 (comment)) When downgrading, a peers.dat with a future version that has a minimum required version larger than the downgraded Bitcoin Core version would cause an InitError. This commit changes this behavior to overwrite the existing peers.dat with a new empty one. ACKs for top commit: prayank23: reACK bitcoin@d41ed32 kallewoof: reACK d41ed32 Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40

Summary: ``` When downgrading, a peers.dat with a future version that has a minimum required version larger than the downgraded version would cause an InitError. This commit changes this behavior to overwrite the existing peers.dat with a new empty one, while creating a backup in peers.dat.bak. ``` Backport of [[bitcoin/bitcoin#24201 | core#24201]]. This can't be used with the recent v4 format update but will make for a better UX if the peers.dat format is changed in the future. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12348

junderw mentioned this pull request Jan 29, 2022

Avoid InitError when downgrading peers.dat #24188

Closed

DrahtBot added the P2P label Jan 29, 2022

kallewoof reviewed Jan 31, 2022

View reviewed changes

luke-jr suggested changes Feb 6, 2022

View reviewed changes

junderw force-pushed the fixes/24188 branch from 96c9d9c to 2948297 Compare February 6, 2022 10:45

luke-jr approved these changes Feb 6, 2022

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

kallewoof reviewed Feb 7, 2022

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

ghost reviewed Feb 8, 2022

View reviewed changes

test/functional/feature_addrman.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 11, 2022

addrman: Log too low compat value #24312

Merged

luke-jr reviewed Feb 20, 2022

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

junderw changed the title ~~p2p: Avoid InitError when downgrading peers.dat~~ WIP: p2p: Avoid InitError when downgrading peers.dat Feb 20, 2022

maflcko added this to the 23.0 milestone Feb 21, 2022

junderw changed the title ~~WIP: p2p: Avoid InitError when downgrading peers.dat~~ p2p: Avoid InitError when downgrading peers.dat Feb 24, 2022

ghost approved these changes Feb 24, 2022

View reviewed changes

junderw force-pushed the fixes/24188 branch from 1e2f0f0 to d41ed32 Compare February 25, 2022 00:54

ghost approved these changes Feb 25, 2022

View reviewed changes

kallewoof reviewed Feb 25, 2022

View reviewed changes

maflcko merged commit b00b60e into bitcoin:master Feb 25, 2022

junderw deleted the fixes/24188 branch February 25, 2022 11:34

bitcoin locked and limited conversation to collaborators Feb 25, 2023

Conversation

junderw commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junderw commented Jan 29, 2022

Uh oh!

maflcko commented Jan 29, 2022

Uh oh!

junderw commented Jan 29, 2022

Uh oh!

junderw commented Jan 30, 2022

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

junderw commented Feb 6, 2022

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

maflcko commented Feb 21, 2022

Uh oh!

michaelfolkson commented Feb 24, 2022

Uh oh!

junderw commented Feb 24, 2022

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junderw commented Feb 24, 2022

Uh oh!

ghost commented Feb 24, 2022

Uh oh!

junderw commented Feb 25, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

michaelfolkson commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

junderw commented Jan 29, 2022 •

edited

Loading

DrahtBot commented Feb 11, 2022 •

edited

Loading

ghost left a comment •

edited by ghost

Loading