Skip to content

Fix (inverse) meaning of -persistmempool #23061

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
maflcko merged 2 commits into from
Sep 27, 2021
Merged

Fix (inverse) meaning of -persistmempool #23061

maflcko merged 2 commits into from
Sep 27, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Sep 22, 2021

Passing -persistmempool is currently treated as -nopersistmempool

@laanwj
Copy link
Member

laanwj commented Sep 22, 2021

Any idea when this regressed, or has this been the case since the option was introduced?

@maflcko
Copy link
Member Author

maflcko commented Sep 22, 2021

I haven't checked, but I remember seeing this bug (#23062) since I started working on Bitcoin Core, though I might be mistaken.

@laanwj
Copy link
Member

laanwj commented Sep 22, 2021

Ok adding backport label for 0.21.x as well then. That seems far enough back.

@ryanofsky ryanofsky mentioned this pull request Sep 22, 2021
Open
ryanofsky
ryanofsky approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

I think change of behavior for -persistmempool should be noted in release notes, but otherwise code review ACK faff17b

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 22, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22976 (scripted-diff: Rename overloaded int GetArg to GetIntArg by ryanofsky)

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.

@DrahtBot DrahtBot mentioned this pull request Sep 22, 2021
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Sep 23, 2021

Tested ACK faff17b

Verified that cherry-picking just the test change fails on master.

Any idea when this regressed, or has this been the case since the option was introduced?

Probably since it was introduced in #9966. Fortunately, I think it's unlikely that anyone would be using -persistmempool since the default is true.

@maflcko
Copy link
Member Author

maflcko commented Sep 23, 2021

it's unlikely that anyone would be using -persistmempool since the default is true.

Indeed. Also, there is no bug report. So maybe we can skip the backport, as the bugfix is also a (silent) behaviour change.

jonatack
jonatack reviewed Sep 23, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Nice find.

@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@bitcoin bitcoin deleted a comment from blockskillally Sep 23, 2021
@maflcko maflcko force-pushed the 2109-fixArgParse branch 2 times, most recently from fabd873 to fa94851 Compare September 24, 2021 06:56
@jnewbery
Copy link
Contributor

jnewbery commented Sep 24, 2021

ACK fa94851fcd

ryanofsky
ryanofsky approved these changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky 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 fa94851fcd1636549c35bc7c3b42348a350d5741. (I thought I acked this previously, but my comments were stuck in draft. Feel free to ignore suggestions below which may be a little late)

hebasto
hebasto approved these changes Sep 25, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK faa9c19, I have reviewed the code and it looks OK, I agree it can be merged.

@jnewbery
Copy link
Contributor

jnewbery commented Sep 27, 2021

reACK faa9c19

@maflcko
Copy link
Member Author

maflcko commented Sep 27, 2021 •
edited
Loading

Removed from 0.21 backport for now, because this is a behaviour change. It is not affecting anyone with high likelyhood.

If it is backported to 22.1, the release notes can be cleared in master.

@maflcko maflcko merged commit 632be55 into bitcoin:master Sep 27, 2021
@maflcko maflcko deleted the 2109-fixArgParse branch September 27, 2021 08:18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 27, 2021
Merge bitcoin#23061: Fix (inverse) meaning of -persistmempool
8dba4ff
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
@luke-jr
Fix (inverse) meaning of -persistmempool
60ef97c 
Github-Pull: bitcoin#23061
Rebased-From: faff17b
@fanquake fanquake mentioned this pull request Oct 14, 2021
Merged
@fanquake
Copy link
Member

fanquake commented Oct 14, 2021

Being backported to 22.x in #23276.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 21, 2021
@fanquake
Fix (inverse) meaning of -persistmempool
bb1e8b0 
Github-Pull: bitcoin#23061
Rebased-From: faff17b
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 21, 2021
@fanquake
doc: Add 23061 release notes
5386e20 
Github-Pull: bitcoin#23061
Rebased-From: faa9c19
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021
@luke-jr
Fix (inverse) meaning of -persistmempool
e8bc4f3 
Github-Pull: bitcoin#23061
Rebased-From: faff17b
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 15, 2022
@fanquake
Fix (inverse) meaning of -persistmempool
db76db7 
Github-Pull: bitcoin#23061
Rebased-From: faff17b
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 15, 2022
@fanquake
doc: Add 23061 release notes
92d44ff 
Github-Pull: bitcoin#23061
Rebased-From: faa9c19
fanquake added a commit that referenced this pull request Mar 1, 2022
@fanquake
Merge #23276: [22.x] Backports for 22.x
9b5f674 
269553f test: Call ceildiv helper with integer (Martin Zumsande)
2f60fc6 ci: Replace soon EOL hirsute with jammy (MarcoFalke)
801b0f0 build: patch qt to explicitly define previously implicit header include (Kittywhiskers Van Gogh)
c768bfa tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
f66bc42 tests: Test for assertion when feerate is rounded down (Andrew Chow)
bd7e08e fees: Always round up fee calculated from a feerate (Andrew Chow)
227ae65 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner)
282863a refactor: include a missing <limits> header in fs.cpp (Joan Karadimov)
7febe4f consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack)
c671c6f the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
a5a1538 build, qt: Fix typo in QtInputSupport check (Hennadii Stepanov)
c95b188 system: skip trying to set the locale on NetBSD (fanquake)
c1cdedd guix: Fix powerpc64(le) dynamic linker name (Carl Dong)
92d44ff doc: Add 23061 release notes (MarcoFalke)
db76db7 Fix (inverse) meaning of -persistmempool (MarcoFalke)
85c78e0 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan)

Pull request description:

  Collecting backports for the 22.1 release. Currently:
  * #23045
  * #23061
  * #23148
  * #22390
  * #22820
  * #22781
  * #22895
  * #23335
  * #23333
  * #22949
  * #23580
  * #23504
  * #24239

ACKs for top commit:
  achow101:
    ACK 269553f

Tree-SHA512: b3a57ea241be7a83488eeb032276f4cf82a0987aada906a82f94a20c4acf9f2397708249dcecbe1c7575e70d09c60b835233d4718af4013c7bc58896c618274c
@reddink reddink mentioned this pull request May 26, 2022
Merged
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 12, 2022
@PiRK
Fix (inverse) meaning of -persistmempool
b969c04 
Summary: This is a backport of [[bitcoin/bitcoin#23061 | core#23061]]

Test Plan:
`ninja all check-all`

I checked that applying the test change without the code changes causes `mempool_persist.py` to fail.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11944
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@jonatack jonatack jonatack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @laanwj @DrahtBot @jnewbery @fanquake @jonatack @ryanofsky @hebasto