Skip to content
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

test doc: tests acceptstalefeeestimates option is only supported on regtest chain #28157

Merged
merged 2 commits into from Aug 22, 2023
Merged

test doc: tests acceptstalefeeestimates option is only supported on regtest chain #28157

merged 2 commits into from Aug 22, 2023

Conversation

ismaelsadeeq
Copy link
Member

This PR Follow up comments from #27622

It test that the new regtest-only option acceptstalefeeestimates is not supported on main, signet and test chains, removes an unnecessary comment, and update fee estimator MAXFILEAGE description comment.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2023 •
edited

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK jonatack
Stale ACK pinheadmz

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27859 (Mempool: persist mempoolminfee accross restarts by ismaelsadeeq)

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 Jul 26, 2023
Closed
@ismaelsadeeq ismaelsadeeq mentioned this pull request Jul 26, 2023
Merged
@pinheadmz
Copy link
Member

Due to the multiple datadirs in the feature_config_args test, this assertion actually does fail in combine_logs.py:

bitcoin/test/functional/combine_logs.py

Line 86 in db7120f

assert next(glob, None) is None # more than one debug.log, should never happen

I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

pinheadmz
pinheadmz reviewed Jul 26, 2023
View reviewed changes
Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

concept ACK db7120f

Good follow up, addressed the TODOs leftover from #27622 well. Left a few comments

test/functional/feature_config_args.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch 2 times, most recently from b79ccd9 to 9bd9632 Compare July 27, 2023 11:43
@ismaelsadeeq
Copy link
Member Author

I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

I think this should be fixed in follow-up PR.

stickies-v
stickies-v reviewed Jul 27, 2023
View reviewed changes
test/functional/feature_config_args.py Outdated Show resolved Hide resolved
test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch from 9bd9632 to d2a9cad Compare July 28, 2023 08:29
glozow
glozow reviewed Aug 1, 2023
View reviewed changes
src/policy/fees.cpp Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch from d2a9cad to ef19d52 Compare August 1, 2023 15:44
jonatack
jonatack reviewed Aug 10, 2023
View reviewed changes
Copy link
Contributor

@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.

Concept ACK

test/functional/feature_config_args.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch 3 times, most recently from b24ffb0 to 530ea15 Compare August 15, 2023 17:34
@ismaelsadeeq
Copy link
Member Author

Due to the multiple datadirs in the feature_config_args test, this assertion actually does fail in combine_logs.py:

bitcoin/test/functional/combine_logs.py

Line 86 in db7120f

assert next(glob, None) is None # more than one debug.log, should never happen

I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

@pinheadmz The update on 530ea15
is using the node[0]'s bitcoin.conf to set the options.

@maflcko
Copy link
Member

maflcko commented Aug 17, 2023

Could rebase for green CI, if still relevant?

@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch from 530ea15 to 8884d5c Compare August 17, 2023 10:08
@ismaelsadeeq
Copy link
Member Author

rebased

jonatack
jonatack reviewed Aug 18, 2023
View reviewed changes
Copy link
Contributor

@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.

Tested ACK 8884d5c

Some non-blocking comments, happy to re-ack if you update.

test/functional/feature_config_args.py Outdated Show resolved Hide resolved
test/functional/feature_config_args.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 07-2023-avoid-serving-stale-fees-follow-up branch from 8884d5c to ee5a036 Compare August 21, 2023 06:19
@jonatack
Copy link
Contributor

ACK ee5a036

glozow
glozow reviewed Aug 22, 2023
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.

utACK ee5a036

@glozow glozow merged commit a84dade into bitcoin:master Aug 22, 2023
15 checks passed
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
@glozow
Merge bitcoin#28157: test doc: tests acceptstalefeeestimates option…
6efaec6 
… is only supported on regtest chain

ee5a036 test: ensure acceptstalefeeestimates is supported only on regtest chain (ismaelsadeeq)
22d5d4b tx fees, policy: doc: update and delete unnecessary comment (ismaelsadeeq)

Pull request description:

  This PR Follow up comments from [bitcoin#27622](bitcoin#27622)

  It test that the new `regtest-only` option `acceptstalefeeestimates` is not supported on [main, signet and test chains](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235218268), removes an unnecessary [comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235204323), and update fee estimator  `MAXFILEAGE` [description comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1233887314).

ACKs for top commit:
  jonatack:
    ACK ee5a036
  glozow:
    utACK ee5a036

Tree-SHA512: 4755f25b08db62f37614ea768272b12580ee0d481fb7fa339379901a6132c66828777c6747d3fe67490ceace3a6ff248bf13bdf65720f6e5ba8642eb762acd3c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatack jonatack jonatack left review comments

@glozow glozow glozow left review comments

@stickies-v stickies-v stickies-v left review comments

@pinheadmz pinheadmz Awaiting requested review from pinheadmz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ismaelsadeeq @DrahtBot @pinheadmz @maflcko @jonatack @glozow @stickies-v