fa6ed82794f4aecbd71667b5491edbbc4eaeaaef doc: update bips.md with buried BIP9 deployments (MarcoFalke)
Pull request description:
Also, remove the activation heights, as they can be retrieved from `./src/chainparams.cpp` (if needed)
ACKs for top commit:
laanwj:
ACK fa6ed82794f4aecbd71667b5491edbbc4eaeaaef, needs backport to 0.19 I guess.
Tree-SHA512: 9c069cc14589a3e2309d76f042677c024a9e14d16dbfccef54c4a2963ca7853d01f042b0237e346538c557591b7553deed9dd811ba64bbd0ced88883d562c59a
d478a472eb0d666e8a762ed8d24fafbabc5f94f3 test: Fix combine_logs.py for AppVeyor build (Martin Zumsande)
Pull request description:
Fixes#16894
This fixes the problem of AppVeyor builds not showing `debug.log` if a functional test fails, because the windows separator `\` doesn't work together with the regex in `combine_logs.py`.
A fix was already attempted in #16896, however, that PR became inactive and was marked "up for grabs", plus it's a really small change.
As suggested by jamesob, this PR uses `pathlib`: For the glob and to convert the path to a posix-style string, it leaves the regex as is (in contrast to #16896 which adjusted the regex).
I tested this locally on Windows and Ubuntu.
Top commit has no ACKs.
Tree-SHA512: 603b4359b6009b6da874c30f69759acda03730ee5747898a0fe957a5fc37ee9ba07858c6aa2169bf4c40521f37e47138e8314d698652ea2760fa0a3f76b890bd
cc556e4a30 Add test for superfluous witness record in deserialization (Gregory Sanders)
25b0786581 Fix missing input template by making minimal tx (Gregory Sanders)
Pull request description:
Adds coverage for changed behavior in https://github.com/bitcoin/bitcoin/pull/14039
ACKs for commit cc556e:
MarcoFalke:
utACK cc556e4a30b4a32eab6722f590489d89b2875de3
Tree-SHA512: 3404c8f75e87503983fac5ae27d877309eb3b902f2ec993762911c71610ca449bef0ed98bd17e029414828025b2713e1bd012e63b2a06497e34f1056acaa6321
fac4e731a8 test: Run invalid_txs.InputMissing test in feature_block (MarcoFalke)
Pull request description:
Tree-SHA512: 24c3f519ba0cf417b66e0df6f5ddc0430e3f419af4705a9c85096da47ff4d8f51487d65b68f3f993800003b3f936d95d8a0bade846e1b45f95b2bdbecc9ebab7
0000ea32656833efa3d2ffd9bab66c88c83334f0 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke)
fa0e5b89cf742df56c6c8f49fe9b3c54d2970a66 Add templated GetRandomDuration<> (MarcoFalke)
Pull request description:
A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter:
```cpp
template <typename D>
D GetRandDur(const D& duration_max)
{
return D{GetRand(duration_max.count())};
}
BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
{
std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
// Want seconds to be in range [0..1hour), but always get zero :((((
BOOST_CHECK_EQUAL(rand_hour.count(), 0);
}
```
Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly.
So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use.
ACKs for top commit:
laanwj:
Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0
promag:
Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0.
jonatack:
ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review:
hebasto:
ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671).
Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7
9f59dde9740d065118bdddde75ef9f4e4603a7b1 rpc: Relock wallet only if most recent callback (João Barbosa)
a2e6db5c4f1bb52a8814102b628e51652493d06a rpc: Add mutex to guard deadlineTimers (João Barbosa)
Pull request description:
This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time
Issue introduced in #18487.
Fixes#18811.
ACKs for top commit:
MarcoFalke:
ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1
ryanofsky:
Code review ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1. No changes since last review except squashing commits.
jonatack:
ACK 9f59dde9740d065118bdddde75ef
Tree-SHA512: 2f7fc03e5ab6037337f2d82dfad432495cc337c77d07c968ee2355105db6292f24543c03456f5402e0e759577a4327758f9372f7ea29de6d56dc3695fda9b379
## Issue being fixed or feature implemented
Now that we have ChainLock sigs in coinbase `VerifyDB()` have to process
them. It works most of the time because usually we simply read
contributions from quorum db
https://github.com/dashpay/dash/blob/develop/src/llmq/quorums.cpp#L385.
However, sometimes these contributions aren't available so we try to
re-build them
https://github.com/dashpay/dash/blob/develop/src/llmq/quorums.cpp#L388.
But by the time we call `VerifyDB()` bls worker threads aren't started
yet, so we keep pushing jobs into worker's queue but it can't do
anything and it halts everything.
backtrace:
```
* frame #0: 0x00007fdd85a2873d libc.so.6`syscall at syscall.S:38
frame #1: 0x0000555c41152921 dashd_testnet`std::__atomic_futex_unsigned_base::_M_futex_wait_until(unsigned int*, unsigned int, bool, std::chrono::duration<long, std::ratio<1l, 1l> >, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) + 225
frame #2: 0x0000555c40e22bd2 dashd_testnet`CBLSWorker::BuildQuorumVerificationVector(Span<std::shared_ptr<std::vector<CBLSPublicKey, std::allocator<CBLSPublicKey> > > >, bool) at atomic_futex.h:102:36
frame #3: 0x0000555c40d35567 dashd_testnet`llmq::CQuorumManager::BuildQuorumContributions(std::unique_ptr<llmq::CFinalCommitment, std::default_delete<llmq::CFinalCommitment> > const&, std::shared_ptr<llmq::CQuorum> const&) const at quorums.cpp:419:65
frame #4: 0x0000555c40d3b9d1 dashd_testnet`llmq::CQuorumManager::BuildQuorumFromCommitment(Consensus::LLMQType, gsl::not_null<CBlockIndex const*>) const at quorums.cpp:388:37
frame #5: 0x0000555c40d3c415 dashd_testnet`llmq::CQuorumManager::GetQuorum(Consensus::LLMQType, gsl::not_null<CBlockIndex const*>) const at quorums.cpp:588:37
frame #6: 0x0000555c40d406a9 dashd_testnet`llmq::CQuorumManager::ScanQuorums(Consensus::LLMQType, CBlockIndex const*, unsigned long) const at quorums.cpp:545:64
frame #7: 0x0000555c40937629 dashd_testnet`llmq::CSigningManager::SelectQuorumForSigning(Consensus::LLMQParams const&, llmq::CQuorumManager const&, uint256 const&, int, int) at signing.cpp:1038:90
frame #8: 0x0000555c40937d34 dashd_testnet`llmq::CSigningManager::VerifyRecoveredSig(Consensus::LLMQType, llmq::CQuorumManager const&, int, uint256 const&, uint256 const&, CBLSSignature const&, int) at signing.cpp:1061:113
frame #9: 0x0000555c408e2d43 dashd_testnet`llmq::CChainLocksHandler::VerifyChainLock(llmq::CChainLockSig const&) const at chainlocks.cpp:559:53
frame #10: 0x0000555c40c8b09e dashd_testnet`CheckCbTxBestChainlock(CBlock const&, CBlockIndex const*, llmq::CChainLocksHandler const&, BlockValidationState&) at cbtx.cpp:368:47
frame #11: 0x0000555c40cf75db dashd_testnet`ProcessSpecialTxsInBlock(CBlock const&, CBlockIndex const*, CMNHFManager&, llmq::CQuorumBlockProcessor&, llmq::CChainLocksHandler const&, Consensus::Params const&, CCoinsViewCache const&, bool, bool, BlockValidationState&, std::optional<MNListUpdates>&) at specialtxman.cpp:202:60
frame #12: 0x0000555c40c00a47 dashd_testnet`CChainState::ConnectBlock(CBlock const&, BlockValidationState&, CBlockIndex*, CCoinsViewCache&, bool) at validation.cpp:2179:34
frame #13: 0x0000555c40c0e593 dashd_testnet`CVerifyDB::VerifyDB(CChainState&, CChainParams const&, CCoinsView&, CEvoDB&, int, int) at validation.cpp:4789:41
frame #14: 0x0000555c40851627 dashd_testnet`AppInitMain(std::variant<std::nullopt_t, std::reference_wrapper<NodeContext>, std::reference_wrapper<WalletContext>, std::reference_wrapper<CTxMemPool>, std::reference_wrapper<ChainstateManager>, std::reference_wrapper<CBlockPolicyEstimator>, std::reference_wrapper<LLMQContext> > const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*) at init.cpp:2098:50
frame #15: 0x0000555c4082fe11 dashd_testnet`AppInit(int, char**) at bitcoind.cpp:145:54
frame #16: 0x0000555c40823c64 dashd_testnet`main at bitcoind.cpp:173:20
frame #17: 0x00007fdd85934083 libc.so.6`__libc_start_main(main=(dashd_testnet`main at bitcoind.cpp:160:1), argc=3, argv=0x00007ffcb8ca5b88, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007ffcb8ca5b78) at libc-start.c:308:16
frame #18: 0x0000555c4082f27e dashd_testnet`_start + 46
```
Fixes#5741
## What was done?
Start LLMQContext early. Alternative solution could be moving bls worker
Start/Stop into llmq context ctor/dtor.
## How Has This Been Tested?
I had a node with that issue. This patch fixed it.
## Breaking Changes
Not sure, hopefully none.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Now that we have ChainLock sigs in coinbase `VerifyDB()` have to process
them. It works most of the time because usually we simply read
contributions from quorum db
https://github.com/dashpay/dash/blob/develop/src/llmq/quorums.cpp#L385.
However, sometimes these contributions aren't available so we try to
re-build them
https://github.com/dashpay/dash/blob/develop/src/llmq/quorums.cpp#L388.
But by the time we call `VerifyDB()` bls worker threads aren't started
yet, so we keep pushing jobs into worker's queue but it can't do
anything and it halts everything.
backtrace:
```
* frame #0: 0x00007fdd85a2873d libc.so.6`syscall at syscall.S:38
frame #1: 0x0000555c41152921 dashd_testnet`std::__atomic_futex_unsigned_base::_M_futex_wait_until(unsigned int*, unsigned int, bool, std::chrono::duration<long, std::ratio<1l, 1l> >, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) + 225
frame #2: 0x0000555c40e22bd2 dashd_testnet`CBLSWorker::BuildQuorumVerificationVector(Span<std::shared_ptr<std::vector<CBLSPublicKey, std::allocator<CBLSPublicKey> > > >, bool) at atomic_futex.h:102:36
frame #3: 0x0000555c40d35567 dashd_testnet`llmq::CQuorumManager::BuildQuorumContributions(std::unique_ptr<llmq::CFinalCommitment, std::default_delete<llmq::CFinalCommitment> > const&, std::shared_ptr<llmq::CQuorum> const&) const at quorums.cpp:419:65
frame #4: 0x0000555c40d3b9d1 dashd_testnet`llmq::CQuorumManager::BuildQuorumFromCommitment(Consensus::LLMQType, gsl::not_null<CBlockIndex const*>) const at quorums.cpp:388:37
frame #5: 0x0000555c40d3c415 dashd_testnet`llmq::CQuorumManager::GetQuorum(Consensus::LLMQType, gsl::not_null<CBlockIndex const*>) const at quorums.cpp:588:37
frame #6: 0x0000555c40d406a9 dashd_testnet`llmq::CQuorumManager::ScanQuorums(Consensus::LLMQType, CBlockIndex const*, unsigned long) const at quorums.cpp:545:64
frame #7: 0x0000555c40937629 dashd_testnet`llmq::CSigningManager::SelectQuorumForSigning(Consensus::LLMQParams const&, llmq::CQuorumManager const&, uint256 const&, int, int) at signing.cpp:1038:90
frame #8: 0x0000555c40937d34 dashd_testnet`llmq::CSigningManager::VerifyRecoveredSig(Consensus::LLMQType, llmq::CQuorumManager const&, int, uint256 const&, uint256 const&, CBLSSignature const&, int) at signing.cpp:1061:113
frame #9: 0x0000555c408e2d43 dashd_testnet`llmq::CChainLocksHandler::VerifyChainLock(llmq::CChainLockSig const&) const at chainlocks.cpp:559:53
frame #10: 0x0000555c40c8b09e dashd_testnet`CheckCbTxBestChainlock(CBlock const&, CBlockIndex const*, llmq::CChainLocksHandler const&, BlockValidationState&) at cbtx.cpp:368:47
frame #11: 0x0000555c40cf75db dashd_testnet`ProcessSpecialTxsInBlock(CBlock const&, CBlockIndex const*, CMNHFManager&, llmq::CQuorumBlockProcessor&, llmq::CChainLocksHandler const&, Consensus::Params const&, CCoinsViewCache const&, bool, bool, BlockValidationState&, std::optional<MNListUpdates>&) at specialtxman.cpp:202:60
frame #12: 0x0000555c40c00a47 dashd_testnet`CChainState::ConnectBlock(CBlock const&, BlockValidationState&, CBlockIndex*, CCoinsViewCache&, bool) at validation.cpp:2179:34
frame #13: 0x0000555c40c0e593 dashd_testnet`CVerifyDB::VerifyDB(CChainState&, CChainParams const&, CCoinsView&, CEvoDB&, int, int) at validation.cpp:4789:41
frame #14: 0x0000555c40851627 dashd_testnet`AppInitMain(std::variant<std::nullopt_t, std::reference_wrapper<NodeContext>, std::reference_wrapper<WalletContext>, std::reference_wrapper<CTxMemPool>, std::reference_wrapper<ChainstateManager>, std::reference_wrapper<CBlockPolicyEstimator>, std::reference_wrapper<LLMQContext> > const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*) at init.cpp:2098:50
frame #15: 0x0000555c4082fe11 dashd_testnet`AppInit(int, char**) at bitcoind.cpp:145:54
frame #16: 0x0000555c40823c64 dashd_testnet`main at bitcoind.cpp:173:20
frame #17: 0x00007fdd85934083 libc.so.6`__libc_start_main(main=(dashd_testnet`main at bitcoind.cpp:160:1), argc=3, argv=0x00007ffcb8ca5b88, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007ffcb8ca5b78) at libc-start.c:308:16
frame #18: 0x0000555c4082f27e dashd_testnet`_start + 46
```
Fixes#5741
## What was done?
Start LLMQContext early. Alternative solution could be moving bls worker
Start/Stop into llmq context ctor/dtor.
## How Has This Been Tested?
I had a node with that issue. This patch fixed it.
## Breaking Changes
Not sure, hopefully none.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Missing changes in https://github.com/dashpay/dash/pull/5736
The prior backport of https://github.com/bitcoin/bitcoin/pull/19438 has
been needed to this particular changes: drop the mutex `cs_llmq_vbc`.
This mutex can potentially cause deadlock such as:
```
'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
(2) 'cs_llmq_vbc' in llmq/utils.cpp:704 (in thread 'main')
'm_mutex' in versionbits.cpp:253 (in thread 'main')
(1) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Current lock order is:
'cs_Shutdown' in init.cpp:220 (TRY) (in thread 'shutoff')
(1) 'cs_main' in init.cpp:328 (in thread 'shutoff')
(2) 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff')
Assertion failed: detected inconsistent lock order for 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff'), details in debug log.
```
## What was done?
Drop `cs_llmq_vbc` mutex from llmq/utils
## How Has This Been Tested?
Re-started app several times -> no other deadlock happens.
## Breaking Changes
N/A
## 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
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Fixes a bug we missed in #5736
## What was done?
Use all collected indexes, not just the last one
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
Prior required changes: bitcoin/bitcoin#19438 from
https://github.com/dashpay/dash/pull/5740
## Issue being fixed or feature implemented
Assertion failure:
```
assertion: ok
file: evo/mnhftx.cpp, line: 287
function: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*)
No debug information available for stacktrace. You should add debug information and then run: dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5e3ifzxgzlsoruw63ramzqws3dvojstucraebqxg43foj2gs33ohiqg62ykeaqgm2lmmu5cazlwn4xw23timz2hqltdobycyidmnfxgkoragi4docraebthk3tdoruw63r2ebawe43uojqwg5cfjbde2ylomftwk4r2hjjwsz3omfwhgicdjvheqrsnmfxgcz3foi5dur3fordhe33ninqwg2dffbrw63ttoqqegqtmn5rwwslomrsxqkrjbyrelhyaaaaaaaedgkiaaaaaaaadsm4qaaaaaaaa7gpyqaaaaaaaa2njraaaaaaaadkl3caaaaaaaabhxznqaaaaaaadqa2eaaaaaaaax33twaaaaaaabwaihqaaaaaaac7yooqbaaaaaahba45qcaaaaaacwkz2aeaaaaaaeitgeaiaaaaaaaa=
dash-qt: evo/mnhftx.cpp:287: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*): Assertion `ok' failed.
```
This can happen in case if Dash Core has been update from v19 (or
earlier v20.alphaX) to v20.0.0 after v20 activation without re-indexing
## What was done?
`CMNHFManager` is visiting missing blocks recursively until reach first
v20 block or first block actually saved in evoDb.
Without changes from bitcoin/bitcoin#19438 there's an other issue:
```
2023-11-27T11:12:10Z POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_main' in llmq/instantsend.cpp:459 (in thread 'isman')
(1) 'cs_llmq_vbc' in llmq/utils.cpp:711 (in thread 'isman')
Current lock order is:
'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
(1) 'cs_llmq_vbc' in llmq/utils.cpp:719 (in thread 'main')
(2) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Assertion failed: detected inconsistent lock order for 'cs_main' in node/blockstorage.cpp:77 (in thread 'main'), details in debug log.
2023-11-27T11:12:10Z Posix Signal: Aborted
```
## How Has This Been Tested?
Run unit/functional test; run dash-qt on my local backup of problematic
storage (succeed without error); reindex testnet.
## Breaking Changes
N/A
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
When DKG data recovery is triggered by `qgetdata` the data we use to
construct `qdata` reply is actually the one handled by
`CDKGSessionManager`, not by `CQuorumManager`. Not storing the data long
enough in `CDKGSessionManager` will result in this data simply not being
recoverable.
Also, the formula in `CDKGSessionManager::CleanupOldContributions()` is
broken for quorums which use rotation (the depth is way too large).
## What was done?
Fix both issues by redefining `keepOldKeys` and aligning key storage
depths in both modules.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Avoid a crash on -reindex-chainstate.
## What was done?
`ResetBlockFailureFlags` is crashing when `m_chain.Tip()` is null. Call
`ResetBlockFailureFlags` inside `if (!is_coinsview_empty(chainstate))
{...}` block - we know `m_chain.Tip()` is not null there.
## How Has This Been Tested?
Try running a node with `-reindex-chainstate` cmd-line param w/ and
w/out this patch.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
https://github.com/dashpay/dash/pull/4788#discussion_r854468664
noticed while working on #5731
## What was done?
## How Has This Been Tested?
run a node, check logs - there is a meaningful time span between `start`
and `done` now and not just zeros all the time.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
## What was done?
## How Has This Been Tested?
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
## Issue being fixed or feature implemented
Should fix compilation errors like
```
masternode/meta.cpp:43:9: error: call to member function 'pushKV' is ambiguous
ret.pushKV("lastOutboundAttemptElapsed", now - lastOutboundAttempt);
^~
masternode/meta.cpp:45:9: error: call to member function 'pushKV' is ambiguous
ret.pushKV("lastOutboundSuccessElapsed", now - lastOutboundSuccess);
^~
```
on FreeBSD + clang-15
kudos to @MrDefacto for finding the issue and testing the fix
## What was done?
Specify `now` variable type explicitly instead of relying on `auto`
## How Has This Been Tested?
MrDefacto confirmed it compiles with no issues on FreeBSD now
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Should fix crashes like
```
: Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
Assertion failure:
assertion: globalInstance == nullptr
file: mnhftx.cpp, line: 43
function: CMNHFManager
0#: (0x105ADA27C) stacktraces.cpp:629 - __assert_rtn
1#: (0x104945794) mnhftx.cpp:43 - CMNHFManager::CMNHFManager(CEvoDB&)
2#: (0x10499DA90) compressed_pair.h:40 - std::__1::__unique_if<CMNHFManager>::__unique_single std::__1::make_unique[abi:v15006]<CMNHFManager, CEvoDB&>(CEvoDB&)
3#: (0x10499753C) init.cpp:1915 - AppInitMain(std::__1::variant<std::__1::nullopt_t, std::__1::reference_wrapper<NodeContext>, std::__1::reference_wrapper<WalletContext>, std::__1::reference_wrapper<CTxMemPool>, std::__1::reference_wrapper<ChainstateManager>, std::__1::reference_wrapper<CBlockPolicyEstimator>, std::__1::reference_wrapper<LLMQContext>> const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)
```
## What was done?
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
I was probably using `GITHUB_REPOSITORY` incorrectly, let's try it this
way
#5724 follow-up
## What was done?
## How Has This Been Tested?
n/a
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Missing changes in https://github.com/dashpay/dash/pull/5736
The prior backport of https://github.com/bitcoin/bitcoin/pull/19438 has
been needed to this particular changes: drop the mutex `cs_llmq_vbc`.
This mutex can potentially cause deadlock such as:
```
'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
(2) 'cs_llmq_vbc' in llmq/utils.cpp:704 (in thread 'main')
'm_mutex' in versionbits.cpp:253 (in thread 'main')
(1) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Current lock order is:
'cs_Shutdown' in init.cpp:220 (TRY) (in thread 'shutoff')
(1) 'cs_main' in init.cpp:328 (in thread 'shutoff')
(2) 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff')
Assertion failed: detected inconsistent lock order for 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff'), details in debug log.
```
## What was done?
Drop `cs_llmq_vbc` mutex from llmq/utils
## How Has This Been Tested?
Re-started app several times -> no other deadlock happens.
## Breaking Changes
N/A
## 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
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
When DKG data recovery is triggered by `qgetdata` the data we use to
construct `qdata` reply is actually the one handled by
`CDKGSessionManager`, not by `CQuorumManager`. Not storing the data long
enough in `CDKGSessionManager` will result in this data simply not being
recoverable.
Also, the formula in `CDKGSessionManager::CleanupOldContributions()` is
broken for quorums which use rotation (the depth is way too large).
## What was done?
Fix both issues by redefining `keepOldKeys` and aligning key storage
depths in both modules.
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Fixes a bug we missed in #5736
## What was done?
Use all collected indexes, not just the last one
## 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
facd56750c8a6aee88eeef75d8c8233778d35757 scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (MarcoFalke)
Pull request description:
No longer needed, as it wouldn't help to debug this issue. See https://github.com/bitcoin/bitcoin/pull/22472#issuecomment-882692900
ACKs for top commit:
fanquake:
ACK facd56750c8a6aee88eeef75d8c8233778d35757
Tree-SHA512: 13352b3529c43d6e65ab127134b32158d3072dc2fbbb326fea9adfeada5a8610d0477ea75748b8b68e7abb3b9869a989df3a3169e92bdd458053d64bae6ed379
e46287853f3a41c3f0772d3448d8df4ea01a156f build: remove --enable-determinism configure option (fanquake)
Pull request description:
This was added by me a while back, with the intention of expanding what this did. That hasn't happened, and this hasn't gained much use. There's also been some discussion of some configure option fatigue, so just remove it for now. Note that `-Wl,--no-insert-timestamp` is also already used in the Guix build.
ACKs for top commit:
MarcoFalke:
review ACK e46287853f3a41c3f0772d3448d8df4ea01a156f
jarolrod:
Code Review ACK e46287853f3a41c3f0772d3448d8df4ea01a156f
Tree-SHA512: ac976f88203eca2a49e296a98693dbe53330e0cb0e273c5ff1fcded30daeb6070cc5beeae35cf9acfdc2279cd64c274d5aeb588aef077aa9bfde39bb23570491
7ad414f4bfa74595ee5726e66f3527045c02a977 doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4dd530549d37c43da52c923ac3b2af1a03 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1adfb07b9b466173166dc2e53ace540e4b32 refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)
Pull request description:
I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.
Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).
This is a pretty simple change.
Related to: https://github.com/bitcoin/bitcoin/issues/21766
See also: https://github.com/google/leveldb/issues/142#issuecomment-414418135
ACKs for top commit:
MarcoFalke:
review ACK 7ad414f4bfa74595ee5726e66f3527045c02a977 🔎
jonatack:
re-ACK 7ad414f4bfa74595ee5726e66f3527045c02a977 modulo suggestion
ryanofsky:
Code review ACK 7ad414f4bfa74595ee5726e66f3527045c02a977. Two new commits look good and thanks for clarifying constructor comment
Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
fafd9165e911bf33d6212ca8a613b71878c82449 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)
Pull request description:
Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)
ACKs for top commit:
jamesob:
crACK fafd9165e9
Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
8f7704d0321a71c1691837a6bd3b4e05f84d3031 build: improve detection of eBPF support (fanquake)
Pull request description:
Just checking for the `sys/sdt.h` header isn't enough, as systems like macOS have the header, but it doesn't actually have the `DTRACE_PROBE*` probes, which leads to [compile failures](https://github.com/bitcoin/bitcoin/pull/22006#issuecomment-859559004). The contents of `sys/sdt.h` in the macOS SDK is:
```bash
#ifndef _SYS_SDT_H
#define _SYS_SDT_H
/*
* This is a wrapper header that wraps the mach visible sdt.h header so that
* the header file ends up visible where software expects it to be. We also
* do the C/C++ symbol wrapping here, since Mach headers are technically C
* interfaces.
*
* Note: The process of adding USDT probes to code is slightly different
* than documented in the "Solaris Dynamic Tracing Guide".
* The DTRACE_PROBE*() macros are not supported on Mac OS X -- instead see
* "BUILDING CODE CONTAINING USDT PROBES" in the dtrace(1) manpage
*
*/
#include <sys/cdefs.h>
__BEGIN_DECLS
#include <mach/sdt.h>
__END_DECLS
#endif /* _SYS_SDT_H */
```
The `BUILDING CODE CONTAINING USDT PROBES` section from the dtrace manpage is available [here](https://gist.github.com/fanquake/e56c9866d53b326646d04ab43a8df9e2), and outlines the more involved process of using USDT probes on macOS.
ACKs for top commit:
jb55:
utACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031
practicalswift:
cr ACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031
hebasto:
ACK 8f7704d0321a71c1691837a6bd3b4e05f84d3031, tested on macOS Big Sur 11.4 (20F71) and on Linux Mint 20.1 (x86_64) with depends.
Tree-SHA512: 5f1351d0ac2e655fccb22a5454f415906404fdaa336fd89b54ef49ca50a442c44ab92d063cba3f161cb8ea0679c92ae3cd6cfbbcb19728cac21116247a017df5
faf1af58f85da74f94c6b5f6910c7faf7b47cc88 fuzz: Add Temporary debug assert for oss-fuzz issue (MarcoFalke)
Pull request description:
oss-fuzz is acting weird, so add an earlier assert to help troubleshooting
ACKs for top commit:
practicalswift:
cr ACK faf1af58f85da74f94c6b5f6910c7faf7b47cc88
Tree-SHA512: 85830d7d47cf6b4edfe91a07bd5aa8f7110db0bade8df93868cf276ed04d5dd17e671f769e6a0fb5092012b86aa82bb411fb171411f15746981104ce634c88c1
1111457d7433c2cca1d7e98946f6954e3a0280ef build: Disable deprecated-copy warning only when external warnings are enabled (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/18967
Alternative to https://github.com/bitcoin/bitcoin/pull/22240
ACKs for top commit:
fanquake:
tACK 1111457d7433c2cca1d7e98946f6954e3a0280ef
Tree-SHA512: 0fc826f26ebbeab662fa7eed2a5cc1630c6c4e612deb91734885fc8bae0352be657ec48ae94ff55a984ac36d27b95cea8d947cc5cf408231d56addecf79db83f
91ef8344d4de28b0a659401ef5fefee6c3d9f7ae Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)
Pull request description:
See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and https://github.com/bitcoin/bips/pull/1030.
ACKs for top commit:
practicalswift:
cr ACK 91ef8344d4de28b0a659401ef5fefee6c3d9f7ae
sipa:
ACK 91ef8344d4de28b0a659401ef5fefee6c3d9f7ae. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.
Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
4935ac583bbdc289dd31a1caae3d711edef742b6 qt: Improve GUI responsiveness (Hennadii Stepanov)
75850106aeecfed1d2dc16d8a67ec210c5826a47 qt, macos: Fix GUIUtil::PolishProgressDialog bug (Hennadii Stepanov)
Pull request description:
[`QProgressDialog`](https://doc.qt.io/qt-5/qprogressdialog.html) estimates the time the operation will take (based on time for steps), and only shows itself if that estimate is beyond [`minimumDuration`](https://doc.qt.io/qt-5/qprogressdialog.html#minimumDuration-prop).
The default `minimumDuration` value is [4 seconds](https://doc.qt.io/qt-5/qprogressdialog.html#details), and it could make users think that the GUI is frozen.
This PR sets `minimumDuration` to zero for all progress dialogs, that affects ones in the `WalletControllerActivity` class.
ACKs for top commit:
ryanofsky:
Code review ACK 4935ac583bbdc289dd31a1caae3d711edef742b6. I'm not very familiar with this API but all the changes and explanations make sense and are very clear, and this seems like it should be an improvement.
promag:
Code review ACK 4935ac583bbdc289dd31a1caae3d711edef742b6.
jarolrod:
ACK 4935ac583bbdc289dd31a1caae3d711edef742b6
Tree-SHA512: 2ddd74e7fd87894d341d2439dbaa544d031a350f7f57d4c7e9fbba977dc24080fe60fd7a80a542b1647f1de9091d7fd04a36eab695088d4d75fb836548e99b5f
e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
105941b726c078642e785ecb7b6834ba814381b0 net: use stronger AddLocal() for our I2P address (Vasil Dimov)
Pull request description:
There are two issues:
### 1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` [is used](94f83534e4/src/torcontrol.cpp (L354)) for our `.onion` address and `AddLocal(LOCAL_BIND)` [for our](94f83534e4/src/net.cpp (L2247)) `.b32.i2p` address, the latter being [ignored](94f83534e4/src/net.cpp (L232-L233)) due to `discover=0`.
### 2. Our I2P address removed from local addresses even if specified with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be [added](94f83534e4/src/init.cpp (L1266)) with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as expected. However, if later the I2P proxy is shut down [we do](94f83534e4/src/net.cpp (L2234)) `RemoveLocal()` in order to stop advertising our I2P address (since we have lost I2P connectivity). When the I2P proxy is started and we reconnect to it, restoring the I2P connectivity, [we do](94f83534e4/src/net.cpp (L2247)) `AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which is also what we do with Tor.
ACKs for top commit:
laanwj:
Code review ACK 105941b726c078642e785ecb7b6834ba814381b0
Tree-SHA512: 0c9daf6116b8d9c34ad7e6e9bbff6e8106e94e4394a815d7ae19287aea22a8c7c4e093c8dd8c58a4a1b1412b2575a9b42b8a93672c8d17f11c24508c534506c7
36fb036d25e2a3016b36873456e5a9e6251ffef8 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba5771c81bc0ff2e6523cf9accddae46 test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a01acca64ac21cb13993c6e4f8709f23 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)
Pull request description:
This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.
Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.
The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.
The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.
ACKs for top commit:
theStack:
re-ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8 ☕
vasild:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
hebasto:
ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
f2f2541ee7ad36191515ff351b667fe12a2ab871 remove executable flag for src/net_processing.cpp (Sebastian Falbesoner)
Pull request description:
The file permissions for `src/net_processing.cpp` have been changed in #21713, as discovered by fanquake (https://github.com/bitcoin/bitcoin/pull/21713#issuecomment-822245960). This PR removes the executable flag again.
ACKs for top commit:
kiminuo:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871 :)
jnewbery:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871
promag:
ACK f2f2541ee7ad36191515ff351b667fe12a2ab871.
Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb
7f3a5980c1d54988a707b961fd2ef647cebb4c5b qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)
Pull request description:
Windows and macOS do [not support](https://doc.qt.io/qt-5/qclipboard.html#notes-for-windows-and-macos-users) the global mouse selection.
Fixes#258.
ACKs for top commit:
promag:
Code review ACK 7f3a5980c1d54988a707b961fd2ef647cebb4c5b.
jarolrod:
ACK 7f3a5980c1d54988a707b961fd2ef647cebb4c5b
Tree-SHA512: be2beeef7d25af6f4d4a4548325d8d29f08e4342f499666bc4a670ed468a63195d514077c2cd0dba197e12bd43316fd3e2813cdc0954364b6aa4ae6b90c118bf
732c7bddeb9cd4e9fe80ebb7ee98d0f9fcc6a9d3 tests: Add test for CNetAddr::ToString IPv6 address formatting (RFC 5952) (practicalswift)
Pull request description:
Test that `CNetAddr::ToString` formats IPv6 addresses with zero compression and canonicalisation as described in [RFC 5952 ("A Recommendation for IPv6 Address Text Representation")](https://tools.ietf.org/html/rfc5952).
Solving #21466 will hopefully be trivial with the ability to check zero compression correctness against these tests.
ACKs for top commit:
vasild:
ACK 732c7bddeb9cd4e9fe80ebb7ee98d0f9fcc6a9d3
Tree-SHA512: 31a1378aa435ba4171490a2e15d7280a175292270eb001b47d367e010c6ac9b83420b82bbeab22211f8f500c69e21878047c87adf216263b3420b6bb2a5d2bfb
