backport: merge bitcoin#19905, #19927, #21025, #20749, #21055, partial #19775, #20750, #21270 (prune g_chainman usage) #5235

kwvg · 2023-03-05T07:15:21Z

Additional information

Contains cherry-picked and squashed backport completion 18b48e6 (thanks @UdjinM6!)
- [Bundle 3/n] Prune remaining g_chainman usage in validation functions bitcoin/bitcoin#21055
Contains backports from backport: bitcoin#13868, #16400 #5254 (thanks @knst!)
- Remove unused fScriptChecks parameter from CheckInputs bitcoin/bitcoin#13868
- refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts bitcoin/bitcoin#16400

knst · 2023-03-06T20:48:29Z

src/test/util/setup_common.cpp

@@ -321,12 +322,12 @@ CBlock TestChainSetup::CreateBlock(const std::vector<CMutableTransaction>& txns,
 }
 }

- // Replace mempool-selected txns with just coinbase plus passed-in txns:
- block.vtx.resize(1);
+ Assert(block.vtx.size() == 1);


if DIP3 active, size of vtx can be > 1
May be here should be "vtx.size() >= 1"?

From BlockAssembler:

for (const auto& qcTx : vqcTx) { pblock->vtx.emplace_back(qcTx); pblocktemplate->vTxFees.emplace_back(0); pblocktemplate->vTxSigOps.emplace_back(0); nBlockSize += qcTx->GetTotalSize(); ++nBlockTx; }

The original backport removes an explicit trim that discards everything except the coinbase transaction and replaces it with an assertion, implying that that the sole existence of a coinbase transaction is expected behaviour.

Though, certain Dash tests rely on this explicit trim (specifically block_reward_reallocation_tests) as the test sequence generates four transactions that it then expects CreateBlock to trim, as further down the sequence, assertions fail due to the presence of those transactions it assumes will be trimmed.

test_dash: test/block_reward_reallocation_tests.cpp:205: void block_reward_reallocation_tests::block_reward_reallocation::test_method(): Assertion `deterministicMNManager->GetListAtChainTip().HasMN(tx.GetHash())' failed. unknown location(0): fatal error: in "block_reward_reallocation_tests/block_reward_reallocation": signal: SIGABRT (application abort requested)

I've therefore retained the explicit trim but kept the assertion to prevent merge conflicts down the road. As to whether CreateBlock should account for block_reward_reallocation_tests or vice-versa is subject matter of discussion. @UdjinM6, thoughts?

knst

Note for further re-review: CallOneOf from merge bitcoin#20828 is not applied for addrdb.cpp because it would be removed in bitcoin#22853

src/validation.cpp

src/test/util/setup_common.cpp

src/test/fuzz/addrman.cpp

src/test/fuzz/coins_view.cpp

src/validation.cpp

src/test/fuzz/script_bitcoin_consensus.cpp

src/test/fuzz/net.cpp

src/validation.cpp

knst · 2023-03-14T10:56:16Z

src/validation.cpp

@@ -767,7 +783,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
 // - the node is not behind
 // - the transaction is not dependent on any other transactions in the mempool
 // - the transaction is not a zero fee transaction
- bool validForFeeEstimation = (nFees !=0) && !bypass_limits && IsCurrentForFeeEstimation() && pool.HasNoInputsOf(tx);
+ bool validForFeeEstimation = (nFees !=0) && !bypass_limits && IsCurrentForFeeEstimation(active_chainstate) && pool.HasNoInputsOf(tx);


missing assert: assert(std::addressof(::ChainstateActive()) == std::addressof(m_active_chainstate));

skipped - d8a8163 - it's fine

knst · 2023-03-14T10:57:16Z

src/validation.cpp

- unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(::ChainActive().Tip(), chainparams.GetConsensus());
- if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, currentBlockScriptVerifyFlags, txdata)) {
+ unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(active_chainstate.m_chain.Tip(), chainparams.GetConsensus());
+ if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, currentBlockScriptVerifyFlags, txdata, active_chainstate.CoinsTip())) {


missing asserts:

+ assert(std::addressof(::ChainActive()) == std::addressof(m_active_chainstate.m_chain)); unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus()); + assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(m_active_chainstate.CoinsTip()));

UPD: due to missing commit - d8a8163 - it is fine

knst

partial bitcoin#20750: Prune g_chainman usage in mempool-related validation functions
excludes: - d8a8163 ....

actually this commit is already included so far as I see.

[haven't checked other comments to partial backports, this one is just randomly choosen]

Otherwise, LGTM

## Issue being fixed or feature implemented `GetProjectedMNPayees` wasn't projecting MN payees correctly. ## What was done? HPMNs are now added 4 times before sorting the return list. In addition, the case of last payee being HPMN and having still pending payments is handled. ## How Has This Been Tested? Tested on Masternodes / Next Payment tab on Qt client on Testnet. ## Breaking Changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone

… nodeA thinks it's been 300 seconds but nodeB only thinks it's been 295 for some reason (dashpay#5276) ## Issue being fixed or feature implemented add a bias to IsExpired to avoid potential timing issues where nodeA thinks it's been 300 seconds but nodeB only thinks it's been 295 for some reason ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone

…quorum_data.py` (dashpay#5281) ## Issue being fixed or feature implemented fix `p2p_quorum_data.py` test broken by dashpay#5276 ## What was done? adjust data request expiration timeout in tests ## How Has This Been Tested? `./test/functional/test_runner.py p2p_quorum_data.py` ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone

…n-qt locale handling ca185cf doc: Document differences in bitcoind and bitcoin-qt locale handling (practicalswift) Pull request description: Document differences in `bitcoind` and `bitcoin-qt` locale handling. Since this seems to be the root cause to the locale dependency issues we've seen over the years I thought it was worth documenting :) Note that 1.) `QLocale` (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as `std::to_string`) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization. ACKs for top commit: hebasto: re-ACK ca185cf Tree-SHA512: e51c32f3072c506b0029a001d8b108125e1acb4f2b6a48a6be721ddadda9da0ae77a9b39ff33f9d9eebabe2244c1db09e8502e3e7012d7a5d40d98e96da0dc44

8a22fd0 avoided os-dependant path (Ferdinando M. Ametrano) Pull request description: The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust ACKs for top commit: MarcoFalke: ACK 8a22fd0 Tree-SHA512: 813f27aea33f97c8afac52e716a55fc5d7fb69621023aba99d40df7e1d145e0ec8d1eee49ddd403b219bf0e0e168e0e987b05c78eaef611f744d99bf2fc8bc91

backport: merge bitcoin#18952,18817

…mData (dashpay#5286) ## Issue being fixed or feature implemented should fix "qdata: Already received" discouraging issue the root of the issue is that we remove expired requests on UpdatedBlockTip which is too late sometimes. ## What was done? replacing expired requests with a new one in RequestQuorumData kind of does the same (drops the expired request) but without waiting for UpdatedBlockTip ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [ ] I have assigned this pull request to a milestone

… help texts (dashpay#5290) ## Issue being fixed or feature implemented fix typos in getrawtransaction and decoderawtransaction help texts ## What was done? tweak field name to match https://github.com/dashpay/dash/blob/develop/src/core_write.cpp#L192 ## How Has This Been Tested? ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone

…Inputs

…ng smaller parts

excludes: - fad84b7

…ndex

…tion functions

…unctions Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

…modules contains: - a04aac4 - d0de61b - 46b7f29 - 2afcf24

PastaPastaPasta · 2023-04-04T17:49:46Z

It seems like there's a GitHub bug atm where it shows a bunch of commits here; but if you look at the actual git log after I merged it in everything is correct..

…ports for Darwin targets de4238f build: consolidate reduced export checks (fanquake) 012bdec build: add building libconsensus to end-of-configure output (fanquake) 8f360e3 build: remove ax_gcc_func_attribute macro (fanquake) f054a08 build: remove AX_GCC_FUNC_ATTRIBUTE test for dllimport (fanquake) 7cd0a69 build: test for __declspec(dllexport) in configure (fanquake) 1624e17 build: remove duplicate visibility attribute detection (fanquake) Pull request description: Darwin targets do not have a `protected` visibility function attribute, see [LLVM explanation](https://github.com/llvm/llvm-project/blob/8e9a505139fbef7d2e6e9d0adfe1efc87326f9ef/clang/lib/Basic/Targets/OSTargets.h#L131). This means that the `AX_GCC_FUNC_ATTRIBUTE` check for `visibility` fails: ```bash configure:24513: checking for __attribute__((visibility)) configure:24537: g++ -std=c++11 -o conftest -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -Wl,-headerpad_max_install_names conftest.cpp >&5 conftest.cpp:35:56: warning: target does not support 'protected' visibility; using 'default' [-Wunsupported-visibility] int foo_pro( void ) __attribute__((visibility("protected"))); ^ 1 warning generated. configure:24537: $? = 0 configure:24550: result: no ``` This leads to `EXPORT_SYMBOL` being [defined to nothing](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/src/script/bitcoinconsensus.h#L29), as `HAVE_FUNC_ATTRIBUTE_VISIBILITY` is not defined, and when building with reduced exports, you end up with a libbitcoinconsensus.dylib that doesn't export any `_bitcoinconsensus_*` symbols. ```bash ➜ git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_ ➜ git:(master) ``` We do have a [second check](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/configure.ac#L882) for the `visibility` attribute, which works for Darwin as it's only testing for default visibility, however the result of this check isn't used at all. It was added in dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#5235, it used the results of the added `AX_GCC_FUNC_ATTRIBUTE` calls. This PR removes our usage of the AX_GCC_FUNC_ATTRIBUTE macro entirely, in favour of our own checks in configure. This meant adding a check for `dllexport`, which I've tested as working with both [GCC](https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html) and [Clang](https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#dllexport) when building for Windows. I haven't added an equivalent check for `dllimport`, as we weren't actually using the result of that check, we're just testing that `MSC_VER` was defined before using. With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected: ```bash ./autogen.sh ./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes --enable-reduce-exports make -j8 ... nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_ 000000000000a340 T _bitcoinconsensus_verify_script 00000000000097e0 T _bitcoinconsensus_verify_script_with_amount 000000000000a3c0 T _bitcoinconsensus_version ``` ```python >>> import ctypes >>> consensus = ctypes.CDLL("src/.libs/libbitcoinconsensus.dylib") >>> print(consensus.bitcoinconsensus_version()) 1 >>> exit() ``` TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib? ACKs for top commit: laanwj: Code review ACK de4238f Tree-SHA512: d148f3c55d14dac6e9e5b718cc65bb557bcf6f663218d24bc9044b86281bd5dd3d931ebea79c336a58e8ed50d683218c0a9e75494f2267b91097665043e252ae

kwvg force-pushed the chainman_prune branch from d1a6fd6 to 14e8bdf Compare March 5, 2023 20:10

kwvg mentioned this pull request Mar 5, 2023

backport: merge bitcoin#19806, #21582, #21584, #21270, #21789, #22415, #21525, #21592 (assumeutxo project backports, part 3) #5236

Merged

knst requested changes Mar 6, 2023

View reviewed changes

kwvg force-pushed the chainman_prune branch 3 times, most recently from 536aae4 to f6e8153 Compare March 8, 2023 09:55

kwvg marked this pull request as ready for review March 8, 2023 12:28

kwvg requested review from PastaPastaPasta, knst and UdjinM6 March 8, 2023 12:28

kwvg force-pushed the chainman_prune branch 2 times, most recently from 50f2dbd to 335c412 Compare March 9, 2023 18:23

knst requested changes Mar 10, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

kwvg force-pushed the chainman_prune branch from 335c412 to c9a05f1 Compare March 10, 2023 21:37

kwvg mentioned this pull request Mar 10, 2023

backport: merge bitcoin#21523, #17954, #17905, #17993, #19132, #20494, #19425, #17513, bitcoin-gui#8, partial bitcoin#17938, #18587 (auxiliary backports: part 9) #5246

Merged

kwvg requested a review from knst March 11, 2023 11:11

kwvg force-pushed the chainman_prune branch from c9a05f1 to aa27817 Compare March 12, 2023 21:39

UdjinM6 requested changes Mar 13, 2023

View reviewed changes

kwvg force-pushed the chainman_prune branch 4 times, most recently from 3c3519a to d06a828 Compare March 13, 2023 15:49

kwvg requested a review from UdjinM6 March 13, 2023 15:52

knst requested changes Mar 14, 2023

View reviewed changes

knst mentioned this pull request Mar 15, 2023

backport: bitcoin#13868, #16400 #5254

Closed

2 tasks

kwvg force-pushed the chainman_prune branch 3 times, most recently from 4340fcd to dc8f987 Compare March 16, 2023 08:55

kwvg requested a review from knst March 16, 2023 09:05

knst requested changes Mar 16, 2023

View reviewed changes

ogabrielides and others added 21 commits March 30, 2023 10:06

Merge pull request dashpay#5013 from vijaydasmp/bp21_3

38b42f6

backport: merge bitcoin#18952,18817

merge bitcoin#13868: Remove unused fScriptChecks parameter from Check…

ce3e807

…Inputs

merge bitcoin#16400: refactor: Rewrite AcceptToMemoryPoolWorker() usi…

bd00a6a

…ng smaller parts

merge bitcoin#19905: Remove dead CheckForkWarningConditionsOnNewFork

bce9b6b

merge bitcoin#19927: Reduce direct g_chainman usage

0d7516e

merge bitcoin#21025: Guard all chainstates with cs_main

6648e9f

partial bitcoin#19775: Activate segwit in TestChain100Setup

b0b80d0

excludes: - fad84b7

merge bitcoin#20828: Introduce CallOneOf helper to replace switch-case

431415a

fuzz: remove unused variable in script_bitcoin_consensus

3eb8931

merge bitcoin#20749: Prune g_chainman usage related to ::LookupBlockI…

9d55bd8

…ndex

merge bitcoin#20972: Annotate CTxMemPool::check to require cs_main

228a7fb

merge bitcoin#20750: Prune g_chainman usage in mempool-related valida…

30191be

…tion functions

merge bitcoin#21055: Prune remaining g_chainman usage in validation f…

548e870

…unctions Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

partial bitcoin#21270: Prune g_chainman usage in validation-adjacent …

24a5552

…modules contains: - a04aac4 - d0de61b - 46b7f29 - 2afcf24

PastaPastaPasta dismissed knst’s stale review via 24a5552 April 4, 2023 17:41

PastaPastaPasta merged commit 389f959 into dashpay:develop Apr 4, 2023

UdjinM6 added this to the 20 milestone Apr 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport: merge bitcoin#19905, #19927, #21025, #20749, #21055, partial #19775, #20750, #21270 (prune g_chainman usage) #5235

backport: merge bitcoin#19905, #19927, #21025, #20749, #21055, partial #19775, #20750, #21270 (prune g_chainman usage) #5235

kwvg commented Mar 5, 2023 •

edited

Loading

knst Mar 6, 2023 •

edited

Loading

kwvg Mar 8, 2023

knst left a comment

knst Mar 14, 2023

knst Mar 14, 2023

knst Mar 14, 2023

knst Mar 14, 2023

knst left a comment •

edited

Loading

PastaPastaPasta commented Apr 4, 2023

backport: merge bitcoin#19905, #19927, #21025, #20749, #21055, partial #19775, #20750, #21270 (prune g_chainman usage) #5235

backport: merge bitcoin#19905, #19927, #21025, #20749, #21055, partial #19775, #20750, #21270 (prune g_chainman usage) #5235

Conversation

kwvg commented Mar 5, 2023 • edited Loading

Additional information

knst Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

kwvg Mar 8, 2023

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

knst Mar 14, 2023

Choose a reason for hiding this comment

knst Mar 14, 2023

Choose a reason for hiding this comment

knst Mar 14, 2023

Choose a reason for hiding this comment

knst Mar 14, 2023

Choose a reason for hiding this comment

knst left a comment • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta commented Apr 4, 2023

kwvg commented Mar 5, 2023 •

edited

Loading

knst Mar 6, 2023 •

edited

Loading

knst left a comment •

edited

Loading