4045a6722c884be779e86016313061ac6ff80808 ci: Use cpu=1 for linter (Dhruv Mehta)
739d39022d2959c1114c14a0065daebf4fe812c1 ci: Move linter task to cirrus (Dhruv Mehta)
Pull request description:
Solves #20467: Move linter to Cirrus-CI as Travis-CI.org is shutting down
ACKs for top commit:
MarcoFalke:
ACK 4045a6722c884be779e86016313061ac6ff80808
Tree-SHA512: 9aa7487ac86c91fc68bb584d29134e304dbd46702514a5d47d1ef0de6b877d96d42b7589870fc67ad9a31f5d3a789728446da4418688f336111a9ba0f8de5feb
faf2c6e32e5b27cf936c9b26e6059027dc020374 cirrus: Schedule one task with paid credits for faster CI feedback (MarcoFalke)
Pull request description:
During times of high activity in the repo, the scheduling of Cirrus CI tasks might put them a few hours in the future. This is fine when all the tasks eventually pass. Though for failing tasks, a failure should ideally be shown to the author and reviewer as soon as possible.
Compute credits can be used to schedule immediately: https://cirrus-ci.org/pricing/#compute-credits. Running all tasks with compute credits will probably be more expensive than our previous CI invoice. However, they are also more flexible.
As a start we could enable only a single task and revisit/re-evaluate the next steps in a month.
ACKs for top commit:
laanwj:
Concept ACK faf2c6e32e5b27cf936c9b26e6059027dc020374
fanquake:
ACK faf2c6e32e5b27cf936c9b26e6059027dc020374
practicalswift:
cr ACK faf2c6e32e5b27cf936c9b26e6059027dc020374: patch looks correct
Tree-SHA512: df599e1c4cc0394f7f03413ad29954ddc87b163b02640d8bfc0497a5dc8d237b8c963c1dd9d01ac83c4a044f575300763097dac2ea6d1a4a163a1cece342b743
fa5c4f12f5d017d8dbe7700ca9688189eacbf32b ci: Adjust cirrus ci task names (MarcoFalke)
Pull request description:
The task names are too long for GitHub to display them properly without truncation in the "checks-view". Fix that by using a new naming scheme:
* Native builds don't mention "x86_64 Linux", as it is redundant, they do mention the OS (bionic or focal) in the name suffix
* Cross builds mention the target in the prefix and the OS (always bionic) in the suffix
* the macos native build simply says "macos native"
ACKs for top commit:
practicalswift:
ACK fa5c4f12f5d017d8dbe7700ca9688189eacbf32b: patch looks correct!
hebasto:
ACK fa5c4f12f5d017d8dbe7700ca9688189eacbf32b, I have reviewed the code and it looks OK, I agree it can be merged.
RiccardoMasutti:
ACK fa5c4f1
Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
2b356117e94f9ef27b67a8e98663f5d676f58c11 ci: no-longer exclude feature_block in TSAN job (fanquake)
Pull request description:
The TSAN job is now running on Cirrus.
Increase the allocated memory to the [maximum allowed](https://cirrus-ci.org/guide/linux/#linux-containers).
ACKs for top commit:
jonasschnelli:
utACK 2b356117e94f9ef27b67a8e98663f5d676f58c11 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249
MarcoFalke:
review ACK 2b356117e94f9ef27b67a8e98663f5d676f58c11
Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
fa7a4385d08797876de9a05ae8a6fde18e36bd67 ci: Fix doc typos in .cirrus.yml (MarcoFalke)
fa73674738b0b5a271ae387e2ebd76d747e02d91 ci: Run i686 centos ci config on cirrus (MarcoFalke)
fa1f949a4dc2568456f3ba30bbd8ecbc78a423ca ci: Run nowallet ci config on cirrus (MarcoFalke)
Pull request description:
Travis CI Org is shutting down, so move the configs to cirrus ci
ACKs for top commit:
practicalswift:
ACK fa7a4385d08797876de9a05ae8a6fde18e36bd67: patch looks correct
Tree-SHA512: 1b7125c7f0d2288931fb8c5e90345891d5f7c1f00c4af136afbeb36bd2836f72920a1877dacc7be787e2c8d84de2a733c1ca71931e5c101b222c1fea588619b3
815e4f8026 masternode: protect m_{error,state} with cs (pasta)
136e445abc refactor: pass CActiveMasternodeManager as pointer arg to LLMQContext (Kittywhiskers Van Gogh)
5e0f77747a refactor: pass CActiveMasternodeManager as pointer arg to CJContext (Kittywhiskers Van Gogh)
f171c24a29 refactor: add CActiveMasternodeManager NodeContext alias, use in RPC (Kittywhiskers Van Gogh)
44beb941cb refactor: prefix member variable names with m_ (Kittywhiskers Van Gogh)
73cef4f5f9 refactor: make bls{Pub}KeyOperator member variables instead of pointers (Kittywhiskers Van Gogh)
fbc783635a refactor: make m_info private, get const refs (or copies) from Get*() functions (Kittywhiskers Van Gogh)
1b516ce4ed refactor: use signing helper function instead of passing blsKeyOperator (Kittywhiskers Van Gogh)
33702aca39 refactor: add helper function to decrypt messages with blsKeyOperator (Kittywhiskers Van Gogh)
3eb931b596 refactor: add helper function to sign messages with blsKeyOperator (Kittywhiskers Van Gogh)
3827355cce refactor: move key initialization to InitKeys, define destructor (Kittywhiskers Van Gogh)
e5295dec1f refactor: move activeMasternodeInfo{Cs} into CActiveMasternodeManager (Kittywhiskers Van Gogh)
b8c1f010e7 refactor: avoid accessing active masternode info if not in masternode mode (Kittywhiskers Van Gogh)
9a3c5a3c48 trivial: access activeMasternodeInfo when lock is in scope (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* `CActiveMasternodeManager`, unlike other managers, is _conditionally_ initialized (specifically, when the node is hosting a masternode). This means that checks need to be made to ensure that the conditions needed to initialize the manager are true or that the pointer leads to a valid manager instance.
As the codebase currently checks (and fast-fails) based on the node being in "masternode mode" (`fMasternodeMode`) or not, we will continue with this approach, but with additional assertions _after_ the masternode mode check if the manager exists.
* Though, since `activeMasternodeInfo`(`Cs`) are global variables, they can be accessed _regardless_ of whether the corresponding manager exists. This means some parts of the codebase attempt to fetch information about the (nonexistent) active masternode _before_ determining if it should use the masternode mode path or not (looking at you, `CMNAuth::ProcessMessage`)
Moving them into `CActiveMasternodeManager` meant adding checks _before_ attempting to access information about the masternode, as they would no longer be accessible with dummy values ([here](2110c0c309/src/init.cpp (L1633-L1635))) on account of being part of the conditionally initialized manager.
* In an attempt to opportunistically dereference the manager, `CDKGSessionManager` (accepting a pointer) was dereferencing the manager before passing it to `CDKGSessionHandler`. This was done under the assumption that `CDKGSessionManager` would only ever be initialized in masternode mode.
This is not true. I can confirm that because I spent a few days trying to debug test failures. `CDKGSessionHandler` is initialized in two scenarios:
* In masternode mode
* If the `-watchquorums` flag is enabled
The latter scenario doesn't initialize `CActiveMasternodeManager`.
Furthermore, the DKG round thread is started unconditionally ([here](2110c0c309/src/llmq/context.cpp (L79))) and the `CDKGSessionHandler::StartThreads` > `CDKGSessionHandler::StartThread` > `CDKGSessionHandler::PhaseHandlerThread` > `CDKGSessionHandler::HandleDKGRound` > `CDKGSessionHandler::InitNewQuorum` > `CActiveMasternodeManager::GetProTxHash` call chain reveals an attempt to fetch active masternode information without any masternode mode checks.
This behaviour has now been changed and the thread will only be spun up if in masternode mode.
* Dereferencing so far has been limited to objects that primarily hold data (like `CCoinJoinBroadcastTx` or `CGovernanceObject`) as they should not have knowledge of node's state (that responsibility lies with whatever manager manipulates those objects), perform one-off operations and static functions.
* `activeMasternodeInfo` allowed its members to be read-write accessible to anybody who asked. Additionally, signing and decrypting involved borrowing the operator secret key from the active masternode state to perform those operations.
This behaviour has now been changed. The internal state is now private and accessible read-only as a const ref (or copy) and `Decrypt`/`Sign` functions have been implemented to allow those operations to happen without having another manager access the operator private key in order to do so.
* You cannot combine a `WITH_LOCK` and an `Assert` (in either mutex or accessed value), doing so will cause errors if `-Werror=thread-safety` is enabled. This is why `assert`s are added even when it would intuitively seem that `Assert` would've been more appropriate to use.
## Future Considerations
Currently there are no unit tests that test the functionality of `CActiveMasternodeManager` as it's never initialized in test contexts, breakage had to be found using functional tests. Perhaps some (rudimentary) tests for `CActiveMasternodeManager` may prove to be valuable.
## Breaking Changes
Not _really_. Some behaviour has been modified but nothing that should necessitate updates or upgrades.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 815e4f8026
Tree-SHA512: cbe49ea9e1c35df514e1b40869ee271baef1c348c9d09e4b356e5fc8fe5449cbbe66569258f2d664029faa9a46f711df9bf9e41eb8734c3aefc6cd8e94378948
d5d1a714fb Merge bitcoin/bitcoin#24390: test: Remove suppression no longer needed with headers-only Boost.Test (fanquake)
51630d2e5e Merge bitcoin/bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId (MarcoFalke)
a9b1575fe8 Merge bitcoin/bitcoin#22781: wallet: fix the behavior of IsHDEnabled, return false in case of a blank hd wallet. (Samuel Dobson)
0505229c89 Merge bitcoin/bitcoin#22327: cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
1dc97c7679 Merge bitcoin/bitcoin#22149: test: Add temporary logging to debug #20975 (W. J. van der Laan)
44f91cbc9a Merge #21597: test: Document race:validation_chainstatemanager_tests suppression (fanquake)
c326830f48 Merge bitcoin-core/gui#243: fix issue when disabling the auto-enabled blank wallet checkbox (MarcoFalke)
267f42fd6a Merge #21382: build: Clean remnants of QTBUG-34748 fix (fanquake)
1fcc5f1101 Merge #20540: test: Fix wallet_multiwallet issue on windows (MarcoFalke)
4afbaf2ea1 Merge #20322: test: Fix intermittent issue in wallet_listsinceblock (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of backports
## What was done?
Trivial batch of backports
## How Has This Been Tested?
CI looks good
## Breaking Changes
None
## 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)_
Top commit has no ACKs.
Tree-SHA512: 8eeac54f011eb1111888c745dd56184ac9601de290f2b0f7b7ad02240e8dc1cab5a47fed26bfed2bd6f1066e0710827a3e5b2426f0bf66821cf1cd09099d5160
0d9046586b merge bitcoin#21254: Avoid connecting to real network when running tests (Kittywhiskers Van Gogh)
2f672bdb3a merge bitcoin#20721: Move ping data to net_processing (Kittywhiskers Van Gogh)
0d46acbfef merge bitcoin#21165: Use mocktime in test_seed_peers (Kittywhiskers Van Gogh)
8f40769385 merge bitcoin#19884: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty (Kittywhiskers Van Gogh)
446076d094 test: add missing `dnsseed=0` in configuration (Kittywhiskers Van Gogh)
081d8db4d5 mempool: remove stray boost::optional usage (Kittywhiskers Van Gogh)
bcd383c2d6 merge bitcoin#20724: Cleanup of -debug=net log messages (Kittywhiskers Van Gogh)
8c63868d9b merge bitcoin#19972: Add fuzzing harness for node eviction logic (Kittywhiskers Van Gogh)
18f2dc0865 partial bitcoin#19829: Move block inventory state to net_processing (Kittywhiskers Van Gogh)
ec77bd3ab4 net: move nLast{Block,TX}Time to match upstream location (Kittywhiskers Van Gogh)
f635e4a95f merge bitcoin#20624: Remove nStartingHeight check from block relay (Kittywhiskers Van Gogh)
9f1a3e5f18 merge bitcoin#20477: Add unit testing of node eviction logic (Kittywhiskers Van Gogh)
2d838a627c merge bitcoin#20146: Send post-verack handshake messages at most once (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* [bitcoin#19884](https://github.com/bitcoin/bitcoin/pull/19884) doesn't seem to play nice on its own and necessitated two more backports (namely [bitcoin#21165](https://github.com/bitcoin/bitcoin/pull/21165) and [bitcoin#21254](https://github.com/bitcoin/bitcoin/pull/21254)) before it did.
* Trying to find why that is has been time consuming but there doesn't seem to be a concrete answer. If running the daemon normally (`dashd --regtest --dnsseed=1`), the functionality behaves as expected but the test still fails.
* The closest explanation is that our OOO backports with relation to mocking time could explain why it isn't working as expected due to debug statements I added that always shown the time delta between each "enough time has passed" check was 0 seconds even when the log was advancing forward in time.
* The usage of `dnsseed=0` stems from [bitcoin#16551](https://github.com/bitcoin/bitcoin/pull/16551) (link to diff in commit comment), a backport that was skipped due to complexity. Though, some aspects of the PR have made it with [dash#3946](https://github.com/dashpay/dash/pull/3946).
* [bitcoin#19829](https://github.com/bitcoin/bitcoin/pull/19829) does away with `CConnman::ForEachNode` usage in `PeerManagerImpl::UpdatedBlockTip` ([source](https://github.com/bitcoin/bitcoin/pull/19829/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1318-R1328)). Dash cannot do the same and continues to use `ForEachNode` as we use the `CNode::CanRelay` check, which is not accessible through the `Peer` struct.
* It would be valuable to find values that are Dash-specific (like `m_masternode_connection`) and migrate them to `Peer` to avoid Dash-specific patches and closer alignment with upstream.
## Breaking Changes
Potential change in behaviour in the GUI and RPC.
In RPC, `getpeerinfo` will now display `pingwait` and `startingheight` if `fStateStats` is true (earlier behaviour was unconditional). `startingheight` has been placed below `banscore` (earlier behaviour placed it above). In the GUI (Qt), `peerHeight` and `peerPingWait` are subject to similar conditionality as mentioned earlier.
No changes in protocol or consensus. Changes are primarily related to refactoring, cleaning up, improving networking code and adding a new flag (`-fixedseeds`).
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
re-utACK 0d9046586b
Tree-SHA512: 79eedf47387a8715fc9f20c6bc051d4eae832266454445043e1478dc36daafc1679e002623917af43cf923735217622e3985f664123a1de23fadfdfece7e9b6b
81738d2881253f28b69666ada2a01ebb353f503a test: Remove suppression no longer needed with headers-only Boost.Test (Hennadii Stepanov)
Pull request description:
It appears, that moving to [headers-only](https://github.com/bitcoin/bitcoin/pull/24301) Boost.Test makes the removed suppression unneeded even without [bumping](https://github.com/bitcoin/bitcoin/pull/24383) boost version.
ACKs for top commit:
MarcoFalke:
cr ACK 81738d2881253f28b69666ada2a01ebb353f503a
Tree-SHA512: e60443f79a2e38cc78fceeff5c2956d622e8a10730129f9c27c14aef59bc6fa0894b8011e6191530443bf3165f78da978bc08ad04248ddb65e2da373264afa6a
0bd882b7405414b5355e69a9fdcd7a533e504b6b refactor: remove RecursiveMutex cs_nBlockSequenceId (Sebastian Falbesoner)
Pull request description:
The RecursiveMutex `cs_nBlockSequenceId` is only used at one place in `CChainState::ReceivedBlockTransactions()` to atomically read-and-increment the nBlockSequenceId member:
83daf47898/src/validation.cpp (L2973-L2976)
~~For this simple use-case, we can make the member `std::atomic` instead to achieve the same result (see https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith).~~
~~This is related to #19303. As suggested in the issue, I first planned to change the `RecursiveMutex` to `Mutex` (still possible if the change doesn't get Concept ACKs), but using a Mutex for this simple operation seems to be overkill. Note that at the time when this mutex was introduced (PR #3370, commit 75f51f2a63) `std::atomic` were not used in the codebase yet -- according to `git log -S std::atomic` they have first appeared in 2016 (commit 7e908c7b82), probably also because the compilers didn't support them properly earlier.~~
At this point, the cs_main lock is set, hence we can use a plain int for the member and mark it as guarded by cs_main.
ACKs for top commit:
Zero-1729:
ACK 0bd882b
promag:
Code review ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b.
hebasto:
ACK 0bd882b7405414b5355e69a9fdcd7a533e504b6b
Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9
8733a8e84c4b2e484f6ed6159fcf5f29a360d42e the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
Pull request description:
the result of CWallet::IsHDEnabled() was initialized with true.
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in GUi when reloading a wallet after closing.
ACKs for top commit:
achow101:
ACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
hebasto:
ACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
theStack:
utACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
meshcollider:
utACK 8733a8e84c4b2e484f6ed6159fcf5f29a360d42e
Tree-SHA512: 79b976594f7174d05c29fe3819037ead59aaef27498d95415ceba74d633a8e035f6b03b521000ac3370684a8cb09319d8be1a443ce2d29b3ff4089e399f6b719
fa34cb80248cc39a73fc393f65c3cfc62e849556 cli: Avoid truncating -rpcwaittimeout (MarcoFalke)
Pull request description:
`seconds` is not enough precision to "exactly" store a timestamp n seconds into the future. Improve the precision by using `microseconds`. Fixes#22325
Also, use chrono literals.
ACKs for top commit:
jonatack:
ACK fa34cb80248cc39a73fc393f65c3cfc62e849556 review, debug-built, tested
theStack:
Tested ACK fa34cb80248cc39a73fc393f65c3cfc62e849556
Tree-SHA512: 7158da8545f9998a82bcc8636e04564efdb1e1be43b4288298c151b4df29ad47a2760259eefadd4a01db92ea18a1e017f3febc1cd8c69a4b28c86180229d8c90
faa94961d6e38392ba068381726ed4e033367b03 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
a09e260c15 refacotor: simply RecursiveMutex -> Mutex (pasta)
Pull request description:
## What was done?
This PR is a simpler version of https://github.com/dashpay/dash/pull/5954; it is simply a recursiveMutex -> Mutex PR; as I think some of the scope minimization in the other PR introduced test failures (appears there are undocumented mutex dependancies)
## How Has This Been Tested?
CI TBD
## Breaking Changes
None
## 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)_
ACKs for top commit:
knst:
utACK a09e260c15
Tree-SHA512: 4ef5aa94ecb1a55f30b642d5e0328e2d2a0adcf2ccc2ab6489a576a420c915339be023828785c3243f019bb73ac2cd77be7dd4397e14ec874cba68999a9e54dc
We could use std::optional<std::reference_wrapper<const CActiveMasternodeManager>>
but then we'd also have to contend with accessing the value with mn_activeman.
value().get().
We assert m_mn_activeman is present instead of fast-failing when it isn't
because of the fMasternodeMode fast-fail check. If we are in masternode
mode, m_mn_activeman should point to a valid target. If it doesn't,
something's gone wrong.
Using unique_ptr is a relic of activeMasternodeInfo, as it was accessible
independent of CActiveMasternodeManager. As it is now only accessible
if CActiveMasternodeManager is initialized, we can make it part of its
constructor and make them const.
Removes the assertion and sanity checks to see if key pair is valid.
External logic should not be able to mutate the CActiveMasternodeManager
state (i.e. CActiveMasternodeInfo). Access is brokered through getter
functions.
A later commit will be moving activeMasternodeInfo into activeMasternodeManager
and that is only conditionally initialized if the node is in masternode
mode, which will render access attempts outside of masternode mode invalid.
We need to adjust behaviour to account for that.
