test: doc: follow-up #28368

ismaelsadeeq commented at 3:28 pm on December 6, 2023: member

This is a simple PR that does two things

Fixes #29000 by waiting for the fee estimator to catch up after removeForBlock calls before calling estimateFee in the BlockPolicyEstimates unit test.
Addressed some outstanding review comments from #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

test: wait for fee estimator to catch up before estimating fees 562664d263

DrahtBot commented at 3:28 pm on December 6, 2023: contributor

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 added the label Tests on Dec 6, 2023

in src/test/policyestimator_tests.cpp:73 in 76d0c07ee9 outdated

69@@ -70,7 +70,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
70                                                                                       feeV[j],
71                                                                                       virtual_size,
72                                                                                       entry.nHeight,
73-                                                                                      /* m_from_disconnected_block */ false,
74+                                                                                      /* m_mempool_limit_bypassed */ false,

maflcko commented at 3:33 pm on December 6, 2023:

nit: use the format that clang-tidy understands and that is used elsewhere in the code?

0                                                                                      /*m_mempool_limit_bypassed=*/ false,

ismaelsadeeq commented at 11:39 am on December 8, 2023:

Updated, thanks

maflcko added this to the milestone 27.0 on Dec 6, 2023

in src/test/fuzz/policy_estimator.cpp:51 in 76d0c07ee9 outdated

47@@ -48,7 +48,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
48                 const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
49                                                                entry.GetTxSize(), entry.GetHeight(),
50                                                                /* m_mempool_limit_bypassed */ false,
51-                                                               /* m_submitted_in_package */ false,
52+                                                               /* m_submitted_in_package */ fuzzed_data_provider.ConsumeBool(),

martinus commented at 10:21 am on December 8, 2023:

I’m not sure it is a good idea to have two calls to fuzzed_data_provider in the function call. The problem is that the order of evaluation of function arguments is unspecified: https://en.cppreference.com/w/cpp/language/eval_order So evaluation order can change at any time when compiled. Then the same fuzzing input will produce different results. I’d move the calls in front of NewMempoolTransactionInfo

maflcko commented at 10:36 am on December 8, 2023:

Is there an easy way to catch those somehow in CI?

martinus commented at 11:37 am on December 8, 2023:

Hm, as far as I know there’s no easy way… I once found a bug like that where a deterministic random generator was used, and the unit test was asserting the exact output of an algorithm given a fixed but random input. On Windows the result was different because there the compiler had chosen a different evaluation order.

ismaelsadeeq commented at 11:39 am on December 8, 2023:

Fixed in https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 thank you @martinus

martinus commented at 8:40 am on December 9, 2023:

@maflcko I just looked through the code a bit to find other usages of fuzzed_data_provider that depend on the evaluation order, and there are quite a few of them. E.g.

It would be great if it were possible to find these automatically, but as far as I know that doesn’t exist anywhere. Maybe with a custom clang-tidy plugin for specific classes, (random generator and fuzzer)?

martinus commented at 2:24 pm on December 9, 2023:

I created #29043

ismaelsadeeq commented at 4:20 pm on December 9, 2023:

Thanks @martinus Will review

martinus commented at 10:22 am on December 8, 2023: contributor

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

ismaelsadeeq force-pushed on Dec 8, 2023

martinus commented at 12:03 pm on December 8, 2023: contributor

code review ACK fab46cc

maflcko commented at 12:21 pm on December 8, 2023: member

 0e-argument-comment,-warnings-as-errors]
 1   52 |                                                                /*m_mempool_limit_bypassed=*/false,
 2      |                                                                ^
 3./kernel/mempool_entry.h:256:51: note: 'from_disconnected_block' declared here
 4  256 |                                        const bool from_disconnected_block, const bool submitted_in_package,
 5      |                                                   ^
 6test/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]
 7   54 |                                                                /*m_chainstate_is_current=*/true,
 8      |                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
 9      |                                                                /*chainstate_is_current=*/
10./kernel/mempool_entry.h:257:51: note: 'chainstate_is_current' declared here
11  257 |                                        const bool chainstate_is_current,
12--

DrahtBot added the label CI failed on Dec 8, 2023

ismaelsadeeq force-pushed on Dec 8, 2023

DrahtBot removed the label CI failed on Dec 8, 2023

ismaelsadeeq commented at 4:20 pm on December 9, 2023: member

Thanks for reviewing @martinus @maflcko Forced pushed from https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 to https://github.com/bitcoin/bitcoin/commit/f8ef5dbc8471b711d233e3bec64597498a641252 to fix CI failure compare_changes

in src/kernel/mempool_entry.h:193 in 75f0478e0c outdated

189@@ -190,6 +190,10 @@ class CTxMemPoolEntry
190 
191 using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef;
192 
193+/**

glozow commented at 11:50 am on December 29, 2023:

75f0478e0c435c1ee9242007ee1b391d3175519e this doc seems unnecessary

ismaelsadeeq commented at 11:03 am on January 2, 2024:

All the doc changes here or only TransactionInfo doctring?

glozow commented at 11:14 am on January 2, 2024:

Just the added docstrings. I think it’s self-explanatory that they store information about a tx.

ismaelsadeeq commented at 11:39 am on January 2, 2024:

Thanks fixed now

glozow commented at 12:02 pm on December 29, 2023: member

concept ACK, thanks for fixing the issue

ismaelsadeeq force-pushed on Jan 2, 2024

doc: fix typo and update incorrect comment fcd4296648

tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed`

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.

5615e16b70

test: change `m_submitted_in_package` input to fuzz data provider boolean

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.

b1318dcc56

ismaelsadeeq force-pushed on Jan 2, 2024

glozow commented at 1:56 pm on January 2, 2024: member

utACK b1318dcc56a0181783ee7ddbd388ae878a0efc52

DrahtBot requested review from martinus on Jan 2, 2024

glozow added the label Bug on Jan 2, 2024

glozow removed the label Bug on Jan 2, 2024

martinus commented at 3:45 pm on January 2, 2024: contributor

re-ACK b1318dcc56a0181783ee7ddbd388ae878a0efc52

DrahtBot removed review request from martinus on Jan 2, 2024

glozow assigned glozow on Jan 3, 2024

glozow merged this on Jan 3, 2024

glozow closed this on Jan 3, 2024

glozow unassigned glozow on Jan 3, 2024

ismaelsadeeq deleted the branch on Jun 27, 2024

test: doc: follow-up #28368 #29013

Code Coverage

Reviews

Conflicts