Skip to content

Conversation

@ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented Dec 6, 2023

This is a simple PR that does two things

  1. Fixes Test policyestimator_tests/BlockPolicyEstimates failure #29000 by waiting for the fee estimator to catch up after removeForBlock calls before calling estimateFee in the BlockPolicyEstimates unit test.

  2. Addressed some outstanding review comments from Fee Estimator updates from Validation Interface/CScheduler thread #28368

  • Updated NewMempoolTransactionInfo::m_from_disconnected_block to NewMempoolTransactionInfo::m_mempool_limit_bypassed which now correctly indicates what the boolean does.
  • Changed input of processTransaction's tx_info m_submitted_in_package input from false to fuzz data provider boolean.
  • Fixed some typos, and update incorrect comment

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 6, 2023

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK glozow, martinus

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:

  • #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)

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 added the Tests label Dec 6, 2023
@maflcko maflcko added this to the 27.0 milestone Dec 6, 2023
@martinus
Copy link
Contributor

martinus commented Dec 8, 2023

I can confirm that #29000 is fixed for me in commit 76d0c07ee94b8720964d9d71d462550ecc8b5147.

@ismaelsadeeq ismaelsadeeq force-pushed the 12-2023-28368-follow-up branch 2 times, most recently from f37aba1 to fab46cc Compare December 8, 2023 11:37
@martinus
Copy link
Contributor

martinus commented Dec 8, 2023

code review ACK fab46cc

@maflcko
Copy link
Member

maflcko commented Dec 8, 2023

e-argument-comment,-warnings-as-errors]
   52 |                                                                /*m_mempool_limit_bypassed=*/false,
      |                                                                ^
./kernel/mempool_entry.h:256:51: note: 'from_disconnected_block' declared here
  256 |                                        const bool from_disconnected_block, const bool submitted_in_package,
      |                                                   ^
test/fuzz/policy_estimator.cpp:54:64: error: argument name 'm_chainstate_is_current' in comment does not match parameter name 'chainstate_is_current' [bugprone-argument-comment,-warnings-as-errors]
   54 |                                                                /*m_chainstate_is_current=*/true,
      |                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                /*chainstate_is_current=*/
./kernel/mempool_entry.h:257:51: note: 'chainstate_is_current' declared here
  257 |                                        const bool chainstate_is_current,
--

@ismaelsadeeq
Copy link
Member Author

Thanks for reviewing @martinus @maflcko
Forced pushed from fab46cc to f8ef5db
to fix CI failure
compare_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.

concept ACK, thanks for fixing the issue

@ismaelsadeeq ismaelsadeeq force-pushed the 12-2023-28368-follow-up branch from f8ef5db to 8ff7266 Compare January 2, 2024 11:38
…sed`

The boolean indicates whether the transaction was added without enforcing mempool
fee limits. m_mempool_limit_bypassed is the correct variable name.

Also changes NewMempoolTransactionInfo booleans descriptions to the format that
is consistent with the codebase.
…lean

In reality some mempool transaction might be submitted in a package,
so change m_submitted_in_package to fuzz data provider boolean just like
m_has_no_mempool_parents.
@ismaelsadeeq ismaelsadeeq force-pushed the 12-2023-28368-follow-up branch from 8ff7266 to b1318dc Compare January 2, 2024 11:41
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 b1318dc

@DrahtBot DrahtBot requested a review from martinus January 2, 2024 13:56
@glozow glozow added Bug and removed Bug labels Jan 2, 2024
@martinus
Copy link
Contributor

martinus commented Jan 2, 2024

re-ACK b1318dc

@DrahtBot DrahtBot removed the request for review from martinus January 2, 2024 15:45
@glozow glozow self-assigned this Jan 3, 2024
@glozow glozow merged commit 65c05db into bitcoin:master Jan 3, 2024
@glozow glozow removed their assignment Jan 3, 2024
@ismaelsadeeq ismaelsadeeq deleted the 12-2023-28368-follow-up branch June 27, 2024 11:17
achow101 added a commit that referenced this pull request Sep 4, 2024
01960c5 fuzz: make FuzzedDataProvider usage deterministic (Martin Leitner-Ankerl)

Pull request description:

  There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
  Unfortunately, [the order of evaluation of function arguments is unspecified](https://en.cppreference.com/w/cpp/language/eval_order), and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY

  When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecified evaluation order could be a problem.

  Finding these has been manual work; I grepped the sourcecode for these patterns, and looked at each usage individually. So there is a chance I missed some.

  * `fuzzed_data_provider`
  * `.Consume`
  * `>Consume`
  * `.rand`

  I first discovered this in #29013 (comment). Note that there is a possibility that due to this fix the evaluation order is now different in many cases than when the fuzzing corpus has been created. If that is the case, the fuzzing corpus will have worse coverage than before.

  Update: In list-initialization the order of evaluation is well defined, so e.g. usages in `initializer_list` or constructors that use `{...}` is ok.

ACKs for top commit:
  achow101:
    ACK 01960c5
  vasild:
    ACK 01960c5
  ismaelsadeeq:
    ACK 01960c5

Tree-SHA512: e56d087f6f4bf79c90b972a5f0c6908d1784b3cfbb8130b6b450d5ca7d116c5a791df506b869a23bce930b2a6977558e1fb5115bb4e061969cc40f568077a1ad
@bitcoin bitcoin locked and limited conversation to collaborators Jun 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test policyestimator_tests/BlockPolicyEstimates failure

5 participants