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

ismaelsadeeq commented at 10:15 PM on July 25, 2023: 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 commented at 10:15 PM on July 25, 2023: contributor

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.

pinheadmz commented at 3:07 PM on July 26, 2023: member

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

https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86

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.

in test/functional/feature_config_args.py:112 in db7120fbad outdated

 107 | @@ -108,6 +108,29 @@ def test_config_file_parser(self):
 108 |          self.restart_node(0)
 109 |          self.nodes[0].stop_node(expected_stderr=f'Warning: {inc_conf_file_path}:1 Section [testnot] is not recognized.{os.linesep}{inc_conf_file2_path}:1 Section [testnet] is not recognized.')
 110 |  
 111 | +        with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
 112 | +            conf.write('')  # clear

pinheadmz commented at 3:23 PM on July 26, 2023:

You don't need to clear here, or on L125 because you are opening the conf file with "w" so the contents of the file get overwritten. Tests pass without the two clear lines, and I checked with extra open/read/print statements that the expected configuration settings were present in the file at each step.

ismaelsadeeq commented at 11:44 AM on July 27, 2023:

Thank you. I removed the clears.

pinheadmz commented at 3:23 PM on July 26, 2023: member

concept ACK db7120fbaddff6037734d8e42d1d002d773f8567

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

ismaelsadeeq force-pushed on Jul 27, 2023

ismaelsadeeq commented at 11:45 AM on July 27, 2023: member

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.

in test/functional/feature_config_args.py:127 in 9bd9632723 outdated

 122 | +        self.log.info("test acceptstalefeeestimates option is not supported on signet chain.")
 123 | +        with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
 124 | +            conf.write('signet=1\n')
 125 | +            conf.write('acceptstalefeeestimates=1\n')
 126 | +        self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={inc_conf_file_path}", "-allowignoredconf"], expected_msg='Error: acceptstalefeeestimates is not supported on signet chain.')
 127 | +

stickies-v commented at 3:16 PM on July 27, 2023:

Can be made quite a bit more DRY:

        self.log.info(f"Test -acceptstalefeeestimates option is not supported on non-regtest chains")
        for chain in ["main", "test", "signet"]:
            with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
                conf.write(f'chain={chain}\n')
                conf.write('acceptstalefeeestimates=1\n')
            self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={inc_conf_file_path}", "-allowignoredconf"], expected_msg=f'Error: acceptstalefeeestimates is not supported on {chain} chain.')

Also don't think this belongs in test_config_file_parser, probably worth adding its own function for?

ismaelsadeeq commented at 8:31 AM on July 28, 2023:

Thank you, I updated with your suggestions

in test/functional/feature_fee_estimation.py:381 in 9bd9632723 outdated

 377 | @@ -378,7 +378,7 @@ def test_acceptstalefeeestimates_option(self):
 378 |          os.utime(fee_dat, (last_modified_time, last_modified_time))
 379 |  
 380 |          # Restart node with -acceptstalefeeestimates option to ensure fee_estimate.dat file is read
 381 | -        self.start_node(0,extra_args=["-acceptstalefeeestimates"])
 382 | +        self.start_node(0, extra_args=["-acceptstalefeeestimates"])

stickies-v commented at 3:17 PM on July 27, 2023:

Would generally avoid unrelated style nits, you're not touching this line (or even file, for that matter) for anything else, so best to just leave as is.

ismaelsadeeq commented at 8:31 AM on July 28, 2023:

Done, thanks

ismaelsadeeq force-pushed on Jul 28, 2023

in src/policy/fees.cpp:558 in d2a9cadf04 outdated

 555 |      }
 556 |  
 557 |      std::chrono::hours file_age = GetFeeEstimatorFileAge();
 558 | -    // fee estimate file must not be too old to avoid wrong fee estimates.
 559 | +    // Serving stale fee estimates can cause transactions to become stuck in the bottom of the mempool in the worst case.
 560 | +    // Don't do that.

glozow commented at 8:00 AM on August 1, 2023:

I think I know what you mean, but imo transactions being stuck is a little bit of a jump from feerate underestimation. Maybe "Serving stale fee estimates may lead to overpaying in fees or not meeting minimum feerate requirements in the worst case." Or just delete this comment, as the code is self-explanatory and there is already a comment above MAX_FILE_AGE.

ismaelsadeeq commented at 3:48 PM on August 1, 2023:

I deleted the comment. Thank you

ismaelsadeeq force-pushed on Aug 1, 2023

in test/functional/feature_config_args.py:381 in ef19d52e25 outdated

 376 | +        inc_conf_file_path = os.path.join(self.nodes[0].datadir, 'include.conf')
 377 | +        for chain in ["main", "test", "signet"]:
 378 | +            with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
 379 | +                conf.write(f'chain={chain}\n')
 380 | +                conf.write('acceptstalefeeestimates=1\n')
 381 | +            self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={inc_conf_file_path}", "-allowignoredconf"], expected_msg=f'Error: acceptstalefeeestimates is not supported on {chain} chain.')

jonatack commented at 10:13 PM on August 10, 2023:

I'm not sure why, and maybe it's an issue with my local environment as the CI is green, but this test passes for me in the previous push 9bd96327232ac9fbb7f9cb0a0545a4410bdd4964 but not with the latest push ef19d52e25407df1a56a76038dd4c95e03e75250; only the signet case passes while the main and test cases raise the following:

AssertionError: [node 0] Expected message "Error: acceptstalefeeestimates is not supported on test chain." does not fully match stderr:
"Error: Unable to start HTTP server. See debug log for details."

jonatack commented at 10:40 PM on August 10, 2023:

Correcting myself, it turns out there isn't a difference between the two pushes. The issue comes from having bitcoind already running on mainnet, testnet, or signet; if yes, the test fails with the message I described. If others see this issue too, would it be feasible to write the test to be independent of bitcoind running on those chains when the test is run?

ismaelsadeeq commented at 7:06 PM on August 14, 2023:

Thank you @jonatack, I was able to recreate the same Error. b24ffb05e42ec13ade193c9d7bea0def4296a4a8 should fix this.

jonatack commented at 10:14 PM on August 14, 2023:

Thanks for looking into it! Running the updated test with mainnet/testnet3/signet nodes running, the mainnet test passed. However, it still fails for the testnet3 and signet cases. (IDK about other people, but I usually have mainnet and one or both of signet and testnet3 running while working on this project and running tests.)

(nit in your last push, this diff was correct before the update, 2 spaces is the norm before a # comment)

-            conf.write('')  # clear
+            conf.write('') # clear

You can verify this using, say, the black python linter:

$ black --code "conf.write('') # clear"
conf.write("")  # clear

ismaelsadeeq commented at 5:44 PM on August 15, 2023:

Thanks, Fix this and the nit in 530ea157ea1c25821711617e5f1da3e95cfdc66d

jonatack commented at 10:16 PM on August 10, 2023: member

Concept ACK

ismaelsadeeq force-pushed on Aug 14, 2023

ismaelsadeeq force-pushed on Aug 15, 2023

ismaelsadeeq commented at 5:47 PM on August 15, 2023: member

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

https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86

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 530ea157ea1c25821711617e5f1da3e95cfdc66d is using the node[0]'s bitcoin.conf to set the options.

DrahtBot added the label CI failed on Aug 15, 2023

maflcko commented at 9:26 AM on August 17, 2023: member

Could rebase for green CI, if still relevant?

ismaelsadeeq force-pushed on Aug 17, 2023

tx fees, policy: doc: update and delete unnecessary comment 22d5d4b2b2

ismaelsadeeq commented at 10:09 AM on August 17, 2023: member

rebased

DrahtBot removed the label CI failed on Aug 17, 2023

in test/functional/feature_config_args.py:378 in 8884d5c605 outdated

 370 | @@ -371,6 +371,18 @@ def test_ignored_default_conf(self):
 371 |              f'is being used instead.') + r"[\s\S]*", env=env, match=ErrorMatch.FULL_REGEX)
 372 |          node.args = node_args
 373 |  
 374 | +    def test_acceptstalefeeestimates_arg_support(self):
 375 | +        self.log.info("Test -acceptstalefeeestimates option support")
 376 | +
 377 | +        conf_file = self.nodes[0].datadir_path / "bitcoin.conf"
 378 | +        chains = {"main":"", "test":"testnet3", "signet":"signet"}

jonatack commented at 7:31 PM on August 18, 2023:

        chains = {"main": "", "test": "testnet3", "signet": "signet"}

$ pycodestyle test/functional/feature_config_args.py
...
test/functional/feature_config_args.py:378:25: E231 missing whitespace after ':'
test/functional/feature_config_args.py:378:36: E231 missing whitespace after ':'
test/functional/feature_config_args.py:378:57: E231 missing whitespace after ':'

or alternatively

for chain, chain_name in {("main", ""), ("test", "testnet3"), ("signet", "signet")}:

ismaelsadeeq commented at 6:20 AM on August 21, 2023:

Thank you, I fixed the nit.

in test/functional/feature_config_args.py:385 in 8884d5c605 outdated

 380 | +            util.write_config(conf_file, n=0, chain=chain_name, extra_config='acceptstalefeeestimates=1\n')
 381 | +            self.nodes[0].assert_start_raises_init_error(expected_msg=f'Error: acceptstalefeeestimates is not supported on {chain} chain.')
 382 | +
 383 | +        util.write_config(conf_file, n=0, chain="regtest")  # Reset to regtest
 384 | +
 385 | +

jonatack commented at 7:32 PM on August 18, 2023:

Can drop extra newline

$ pycodestyle test/functional/feature_config_args.py
...
test/functional/feature_config_args.py:386:5: E303 too many blank lines (2)

(optionally also drop the newlines at lines 376 and 382)

ismaelsadeeq commented at 6:21 AM on August 21, 2023:

I removed the blank lines.

jonatack commented at 7:41 PM on August 18, 2023: member

Tested ACK 8884d5c6059495253dc84e1b984315ddbc78ecd3

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

DrahtBot requested review from pinheadmz on Aug 18, 2023

ismaelsadeeq force-pushed on Aug 21, 2023

test: ensure acceptstalefeeestimates is supported only on regtest chain ee5a0369cc

jonatack commented at 10:41 PM on August 21, 2023: member

ACK ee5a0369cc4305da7b3d26f37677de05ad797e51

glozow commented at 8:16 AM on August 22, 2023: member

utACK ee5a0369cc4305da7b3d26f37677de05ad797e51

glozow merged this on Aug 22, 2023

glozow closed this on Aug 22, 2023

Frank-GER referenced this in commit 6efaec637c on Sep 8, 2023

ismaelsadeeq deleted the branch on Jun 27, 2024

bitcoin locked this on Jun 27, 2025