-
Notifications
You must be signed in to change notification settings - Fork 38.7k
test: doc: follow-up #28368 #29013
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.
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
test: doc: follow-up #28368 #29013
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I can confirm that #29000 is fixed for me in commit 76d0c07ee94b8720964d9d71d462550ecc8b5147. |
f37aba1 to
fab46cc
Compare
|
code review ACK fab46cc |
|
fab46cc to
f8ef5db
Compare
|
Thanks for reviewing @martinus @maflcko |
glozow
left a comment
There was a problem hiding this 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
f8ef5db to
8ff7266
Compare
…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.
8ff7266 to
b1318dc
Compare
glozow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK b1318dc
|
re-ACK b1318dc |
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
This is a simple PR that does two things
Fixes Test
policyestimator_tests/BlockPolicyEstimatesfailure #29000 by waiting for the fee estimator to catch up afterremoveForBlockcalls before callingestimateFeein theBlockPolicyEstimatesunit test.Addressed some outstanding review comments from Fee Estimator updates from Validation Interface/CScheduler thread #28368
NewMempoolTransactionInfo::m_from_disconnected_blocktoNewMempoolTransactionInfo::m_mempool_limit_bypassedwhich now correctly indicates what the boolean does.processTransaction's tx_infom_submitted_in_packageinput from false to fuzz data provider boolean.