backport : Merge bitcoin# #5996
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… peers.dat is empty
a09e260 refacotor: simply RecursiveMutex -> Mutex (pasta) Pull request description: ## What was done? This PR is a simpler version of dashpay#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 a09e260 Tree-SHA512: 4ef5aa94ecb1a55f30b642d5e0328e2d2a0adcf2ccc2ab6489a576a420c915339be023828785c3243f019bb73ac2cd77be7dd4397e14ec874cba68999a9e54dc
…_tests suppression fab1987 test: Document race:validation_chainstatemanager_tests suppression (MarcoFalke) Pull request description: ACKs for top commit: jamesob: ACK bitcoin@fab1987 practicalswift: ACK fab1987 Tree-SHA512: 3f1838b4cf11eba768ce06826cd4b57c9065669b61a5530af44216fc96535ebf37124b47a8de8f72aedf32345157a72d2208cd63214481a9cb56c063f05db5dd
faa9496 test: Add temporary logging to debug bitcoin#20975 (MarcoFalke) Pull request description: to be reverted after a fix ACKs for top commit: laanwj: Code review ACK bitcoin@faa9496 Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
fa34cb8 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 bitcoin#22325 Also, use chrono literals. ACKs for top commit: jonatack: ACK fa34cb8 review, debug-built, tested theStack: Tested ACK fa34cb8 Tree-SHA512: 7158da8545f9998a82bcc8636e04564efdb1e1be43b4288298c151b4df29ad47a2760259eefadd4a01db92ea18a1e017f3febc1cd8c69a4b28c86180229d8c90
…false in case of a blank hd wallet. 8733a8e 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 8733a8e hebasto: ACK 8733a8e theStack: utACK 8733a8e meshcollider: utACK 8733a8e Tree-SHA512: 79b976594f7174d05c29fe3819037ead59aaef27498d95415ceba74d633a8e035f6b03b521000ac3370684a8cb09319d8be1a443ce2d29b3ff4089e399f6b719
0bd882b 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: https://github.com/bitcoin/bitcoin/blob/83daf47898f8a79cb20d20316c64becd564cf54c/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 bitcoin#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 dashpay#3370, commit 75f51f2) `std::atomic` were not used in the codebase yet -- according to `git log -S std::atomic` they have first appeared in 2016 (commit 7e908c7), 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 0bd882b. hebasto: ACK 0bd882b Tree-SHA512: 435271ac8f877074099ddb31436665b500e555f7cab899e5c8414af299b154d1249996be500e8fdeff64e4639bcaf7386e12510b738ec6f20e415e7e35afaea9
…eaders-only Boost.Test 81738d2 test: Remove suppression no longer needed with headers-only Boost.Test (Hennadii Stepanov) Pull request description: It appears, that moving to [headers-only](bitcoin#24301) Boost.Test makes the removed suppression unneeded even without [bumping](bitcoin#24383) boost version. ACKs for top commit: MarcoFalke: cr ACK 81738d2 Tree-SHA512: e60443f79a2e38cc78fceeff5c2956d622e8a10730129f9c27c14aef59bc6fa0894b8011e6191530443bf3165f78da978bc08ad04248ddb65e2da373264afa6a
…coin#20624, bitcoin#19972, bitcoin#20724, bitcoin#19884, bitcoin#21165, bitcoin#20721, bitcoin#21254, partial bitcoin#19829 (networking backports) 0d90465 merge bitcoin#21254: Avoid connecting to real network when running tests (Kittywhiskers Van Gogh) 2f672bd merge bitcoin#20721: Move ping data to net_processing (Kittywhiskers Van Gogh) 0d46acb merge bitcoin#21165: Use mocktime in test_seed_peers (Kittywhiskers Van Gogh) 8f40769 merge bitcoin#19884: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty (Kittywhiskers Van Gogh) 446076d test: add missing `dnsseed=0` in configuration (Kittywhiskers Van Gogh) 081d8db mempool: remove stray boost::optional usage (Kittywhiskers Van Gogh) bcd383c merge bitcoin#20724: Cleanup of -debug=net log messages (Kittywhiskers Van Gogh) 8c63868 merge bitcoin#19972: Add fuzzing harness for node eviction logic (Kittywhiskers Van Gogh) 18f2dc0 partial bitcoin#19829: Move block inventory state to net_processing (Kittywhiskers Van Gogh) ec77bd3 net: move nLast{Block,TX}Time to match upstream location (Kittywhiskers Van Gogh) f635e4a merge bitcoin#20624: Remove nStartingHeight check from block relay (Kittywhiskers Van Gogh) 9f1a3e5 merge bitcoin#20477: Add unit testing of node eviction logic (Kittywhiskers Van Gogh) 2d838a6 merge bitcoin#20146: Send post-verack handshake messages at most once (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * [bitcoin#19884](bitcoin#19884) doesn't seem to play nice on its own and necessitated two more backports (namely [bitcoin#21165](bitcoin#21165) and [bitcoin#21254](bitcoin#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](bitcoin#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](dashpay#3946). * [bitcoin#19829](bitcoin#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 0d90465 Tree-SHA512: 79eedf47387a8715fc9f20c6bc051d4eae832266454445043e1478dc36daafc1679e002623917af43cf923735217622e3985f664123a1de23fadfdfece7e9b6b
d5d1a71 Merge bitcoin#24390: test: Remove suppression no longer needed with headers-only Boost.Test (fanquake) 51630d2 Merge bitcoin#22824: refactor: remove RecursiveMutex cs_nBlockSequenceId (MarcoFalke) a9b1575 Merge bitcoin#22781: wallet: fix the behavior of IsHDEnabled, return false in case of a blank hd wallet. (Samuel Dobson) 0505229 Merge bitcoin#22327: cli: Avoid truncating -rpcwaittimeout (MarcoFalke) 1dc97c7 Merge bitcoin#22149: test: Add temporary logging to debug bitcoin#20975 (W. J. van der Laan) 44f91cb Merge bitcoin#21597: test: Document race:validation_chainstatemanager_tests suppression (fanquake) c326830 Merge bitcoin-core/gui#243: fix issue when disabling the auto-enabled blank wallet checkbox (MarcoFalke) 267f42f Merge bitcoin#21382: build: Clean remnants of QTBUG-34748 fix (fanquake) 1fcc5f1 Merge bitcoin#20540: test: Fix wallet_multiwallet issue on windows (MarcoFalke) 4afbaf2 Merge bitcoin#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
…to CActiveMasternodeManager, create NodeContext alias, reduce globals usage 815e4f8 masternode: protect m_{error,state} with cs (pasta) 136e445 refactor: pass CActiveMasternodeManager as pointer arg to LLMQContext (Kittywhiskers Van Gogh) 5e0f777 refactor: pass CActiveMasternodeManager as pointer arg to CJContext (Kittywhiskers Van Gogh) f171c24 refactor: add CActiveMasternodeManager NodeContext alias, use in RPC (Kittywhiskers Van Gogh) 44beb94 refactor: prefix member variable names with m_ (Kittywhiskers Van Gogh) 73cef4f refactor: make bls{Pub}KeyOperator member variables instead of pointers (Kittywhiskers Van Gogh) fbc7836 refactor: make m_info private, get const refs (or copies) from Get*() functions (Kittywhiskers Van Gogh) 1b516ce refactor: use signing helper function instead of passing blsKeyOperator (Kittywhiskers Van Gogh) 33702ac refactor: add helper function to decrypt messages with blsKeyOperator (Kittywhiskers Van Gogh) 3eb931b refactor: add helper function to sign messages with blsKeyOperator (Kittywhiskers Van Gogh) 3827355 refactor: move key initialization to InitKeys, define destructor (Kittywhiskers Van Gogh) e5295de refactor: move activeMasternodeInfo{Cs} into CActiveMasternodeManager (Kittywhiskers Van Gogh) b8c1f01 refactor: avoid accessing active masternode info if not in masternode mode (Kittywhiskers Van Gogh) 9a3c5a3 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](https://github.com/dashpay/dash/blob/2110c0c30983e8f3da02f11907bfe4e1b5dcd89f/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](https://github.com/dashpay/dash/blob/2110c0c30983e8f3da02f11907bfe4e1b5dcd89f/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 815e4f8 Tree-SHA512: cbe49ea9e1c35df514e1b40869ee271baef1c348c9d09e4b356e5fc8fe5449cbbe66569258f2d664029faa9a46f711df9bf9e41eb8734c3aefc6cd8e94378948
fa7a438 ci: Fix doc typos in .cirrus.yml (MarcoFalke) fa73674 ci: Run i686 centos ci config on cirrus (MarcoFalke) fa1f949 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 fa7a438: patch looks correct Tree-SHA512: 1b7125c7f0d2288931fb8c5e90345891d5f7c1f00c4af136afbeb36bd2836f72920a1877dacc7be787e2c8d84de2a733c1ca71931e5c101b222c1fea588619b3
2b35611 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 2b35611 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249 MarcoFalke: review ACK 2b35611 Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
fa5c4f1 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 fa5c4f1: patch looks correct! hebasto: ACK fa5c4f1, I have reviewed the code and it looks OK, I agree it can be merged. RiccardoMasutti: ACK fa5c4f1 Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
667b6a2 ci: Adjust Cirrus CI task names (follow up) (Hennadii Stepanov) Pull request description: "no depends" implies "only system libs". bitcoin#20545 follow up . ACKs for top commit: practicalswift: ACK 667b6a2 Tree-SHA512: b3bf5dc931b7aeed30932e8d6c5f5b0040e4be5a5676ed23b41675ebad2f8a016b1abb27584b68970ee00bc5b2af7122ca9566f373db5f3a2e38fcf7eee6522a
…faster CI feedback faf2c6e 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 faf2c6e fanquake: ACK faf2c6e practicalswift: cr ACK faf2c6e: patch looks correct Tree-SHA512: df599e1c4cc0394f7f03413ad29954ddc87b163b02640d8bfc0497a5dc8d237b8c963c1dd9d01ac83c4a044f575300763097dac2ea6d1a4a163a1cece342b743
4045a67 ci: Use cpu=1 for linter (Dhruv Mehta) 739d390 ci: Move linter task to cirrus (Dhruv Mehta) Pull request description: Solves bitcoin#20467: Move linter to Cirrus-CI as Travis-CI.org is shutting down ACKs for top commit: MarcoFalke: ACK 4045a67 Tree-SHA512: 9aa7487ac86c91fc68bb584d29134e304dbd46702514a5d47d1ef0de6b877d96d42b7589870fc67ad9a31f5d3a789728446da4418688f336111a9ba0f8de5feb
…in repo facf5e3 ci: Only use credits for pull requests to the main repo (MarcoFalke) Pull request description: No need to spend credits for the `master` branch, because the build shouldn't fail there anyway and it is not time-critical to get a fast feedback. Some other changes: * Disable `stateful` for faster scheduling * Reduce lint memory from 8G to 1G for faster scheduling ACKs for top commit: hebasto: ACK facf5e3, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 4d052e68217086574b9ea3d603cde1b585833c289d47dfed5308ff001d02964dc75ec3b3ebf5b233ccd09c47ad4ff5ba0bef639bf6362d984e7c49fca8fec24b
95487b0 doc: Drop mentions of Travis CI as it is no longer used (Hennadii Stepanov) 09d105e ci: Drop travis_fold feature as Travis CI is no longer used (Hennadii Stepanov) Pull request description: As Travis CI is no longer used, this PR: - drops `travis_fold` feature - drops mentions of Travis CI in docs ACKs for top commit: MarcoFalke: ACK 95487b0 Tree-SHA512: 2e259bb8b1e37bcefc1251737bb2716f06ddb57c490010b373825c4e70f42ca38efae69a2f63f21f577d7cee3725b94097bdddbd313f8ebf499281cf97c53cef
faeb40b ci: Install missing lint packages (MarcoFalke) Pull request description: The cirrus container is vanilla ubuntu, so we need to install the needed packages ACKs for top commit: hebasto: ACK faeb40b, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: e56198108e26ea0ba2a344b1b74bc294652f34e9866cca053a25fb1b83bbd87ea40254c340e5e169fdfcbd4dcb39fdc2078b5157ca729a22a9a1792ec514a33e
3c2478c ci: Print COMMIT_RANGE to the log as it was in Travis CI (Hennadii Stepanov) c123892 ci: Drop Travis-specific workaround for shellcheck (Hennadii Stepanov) 10af252 ci: Drop Travis-specific way to set COMMIT_RANGE variable (Hennadii Stepanov) 93504da ci: Fix COMMIT_RANGE variable value for PRs (Hennadii Stepanov) Pull request description: This PR: - is a bitcoin#20658 and bitcoin#20682 followup - set the `COMMIT_RANGE` variable correctly for PRs - cleans up Travis-specific code - prints COMMIT_RANGE value to the log for convenience as it was in Travis CI ACKs for top commit: MarcoFalke: ACK 3c2478c Tree-SHA512: beb933352b10fd5eb3e66373ddb62439e4f3a03b50fb037ee89fa92c0706cec41d05f2d307f15bb18d1e634e6464f4e123b7e2f88703c8edfd145d8d6eff0b1a
3e15712 Update TSan suppressions (Hennadii Stepanov) Pull request description: It seems possible now to drop some TSan suppressions. Top commit has no ACKs. Tree-SHA512: 94518fd2f3a7168b2989424de0696e42c8f509b833aafbc7e75f4c1180a0b8d9a47f43c50d06b03b26a924643afe86274b2062c9d456c17a68576d19566ed66f
1ef2138 lint: run mypy over contrib/devtools (fanquake) Pull request description: wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given bitcoin#20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations. master (patched to check contrib devtools): ```bash test/lint/lint-python.sh contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str") contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...") contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...") Found 4 errors in 3 files (checked 187 source files) ``` I haven't quite gone as far as to add annotations like ```python CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {... ``` to `symbol-check.py`. ACKs for top commit: laanwj: ACK 1ef2138 Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
…ives, bump to codespell 2.0.0 f3ba916 lint: ignore gitian keys file for spelling linter (Sebastian Falbesoner) da289a6 lint: update list of spelling linter false positives (Sebastian Falbesoner) a0022f1 test: bump codespell linter version to 2.0.0 (Sebastian Falbesoner) Pull request description: This small patch updates the ignore list for the spelling linter script (which uses `codespell`), both removing false-positives that are not relevant anymore and adding new ones. As [suggested by jonatack](bitcoin#20762 (comment), whose last name is now also part of the list :)~~. Also changed the linter script to not check the gitian keys file, as [suggested by hebasto](bitcoin#20817 (comment)). The codespell version used is bumped to most recent version 2.0.0, which is more aware of some terms that were previously needed in the ignorelist for v1.17.1, see bitcoin#20817 (comment). Running spelling linter on master branch (repeated findings in the same file are removed to keep the output short): ``` $ ./test/lint/lint-spelling.sh contrib/gitian-keys/keys.txt:16: Atack ==> Attack doc/developer-notes.md:1284: inout ==> input, in out doc/psbt.md:122: Asend ==> Ascend, as end src/bench/verify_script.cpp:27: Keypair ==> Key pair src/blockencodings.h:30: Unser ==> Under, unset, unsure, user src/compressor.h:65: Unser ==> Under, unset, unsure, user src/core_read.cpp:131: presense ==> presence src/index/disktxpos.h:21: blockIn ==> blocking src/net_processing.h:67: anounce ==> announce src/netaddress.h:486: compatiblity ==> compatibility src/primitives/transaction.h:35: nIn ==> inn, min, bin, nine src/qt/bitcoinunits.cpp:101: nIn ==> inn, min, bin, nine src/rpc/blockchain.cpp:2150: nIn ==> inn, min, bin, nine src/rpc/misc.cpp:198: nIn ==> inn, min, bin, nine src/script/bitcoinconsensus.cpp:81: nIn ==> inn, min, bin, nine src/script/bitcoinconsensus.h:63: nIn ==> inn, min, bin, nine src/script/interpreter.cpp:1279: nIn ==> inn, min, bin, nine src/script/interpreter.h:222: nIn ==> inn, min, bin, nine src/script/sign.cpp:17: nIn ==> inn, min, bin, nine src/script/sign.h:39: nIn ==> inn, min, bin, nine src/serialize.h:181: Unser ==> Under, unset, unsure, user src/signet.cpp:142: nIn ==> inn, min, bin, nine src/test/base32_tests.cpp:17: fo ==> of, for src/test/base64_tests.cpp:17: fo ==> of, for src/test/script_tests.cpp:1509: nIn ==> inn, min, bin, nine src/test/sighash_tests.cpp:27: nIn ==> inn, min, bin, nine src/test/validation_tests.cpp:78: excercise ==> exercise src/undo.h:36: Unser ==> Under, unset, unsure, user src/validation.cpp:1403: nIn ==> inn, min, bin, nine src/validation.h:255: nIn ==> inn, min, bin, nine src/wallet/wallet.cpp:1532: nIn ==> inn, min, bin, nine src/wallet/walletdb.cpp:429: Crypted ==> Encrypted test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary test/functional/wallet_encryption.py:81: crypted ==> encrypted test/functional/wallet_upgradewallet.py:36: fpr ==> for, far, fps ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt ``` Running spelling linter on PR branch: ``` $ ./test/lint/lint-spelling.sh src/core_read.cpp:131: presense ==> presence src/net_processing.h:67: anounce ==> announce src/netaddress.h:486: compatiblity ==> compatibility src/test/validation_tests.cpp:78: excercise ==> exercise src/wallet/walletdb.cpp:429: Crypted ==> Encrypted test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary test/functional/wallet_encryption.py:81: crypted ==> encrypted ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt ``` This list of remaining findings doesn't contain false positives anymore -- the typos are fixed in PR bitcoin#20762. Happy new year! 🍾 ACKs for top commit: hebasto: re-ACK f3ba916, only suggested changes since my [previous](bitcoin#20817 (review)) review. jonatack: ACK f3ba916 I don't know if there are any particular issues with bumping codespell to v2.0.0, but locally running the spelling linter and the cirrus job at https://cirrus-ci.com/task/5004066998714368 both LGTM. Thanks for also verifying and removing the unused words from the ignore list. Tree-SHA512: e92ae6f16c01d4ff3d54f8c3a0ee95e12741f7bfe031d307a785f5cfd8a80525b16b34275f413b914c4a318f5166f9887399c21f2dad9cc7e9be41647042ef37
…sage, backport bitcoin#25550, bitcoin#24624, bitcoin#24406, cleanup some gArgs usage in init 740d25c merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings (Kittywhiskers Van Gogh) 3265b54 merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversion warnings (Kittywhiskers Van Gogh) 65585e6 merge bitcoin#25550: remove note on arm cross-compilation from build-unix.md (Kittywhiskers Van Gogh) fbc6bc8 init: use local ArgsManager variable instead of global when possible (Kittywhiskers Van Gogh) 9ae39f9 qt: drop leftover `boost::function` usage in Qt, drop header from list (Kittywhiskers Van Gogh) c47e9e7 bench: drop leftover `boost::lexical_cast` benches, drop header from list (Kittywhiskers Van Gogh) Pull request description: ## Additional Information When building Dash with `clang-16`, I use the following `CXXFLAGS`: ``` -Werror -Werror=reorder -Werror=thread-safety -Wno-unused-command-line-argument -Wno-deprecated-builtins -Wno-deprecated-volatile -Wno-ambiguous-reversed-operator -Wno-deprecated-enum-enum-conversion ``` The explanations of the `-Wno*` flags are as follows: | Argument | Reason | | -------------------------------------- | ------------------------------------------------------------ | | `-Wno-unused-command-line-argument` | Lingering use of `-static-libstdc++`, a GCC-ism that isn't recognized as a valid argument in Clang (no longer an issue thanks to dashpay#5958, thanks knst!) | | `-Wno-deprecated-builtins` | Due to usage of [deprecated builtins](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/bench/nanobench.h#L104-L110) in `nanobench` that exist even in the [most current version](https://github.com/martinus/nanobench/blob/a5a50c2b33eea2ff1fcb355cacdface43eb42b25/src/include/nanobench.h#L115-L121) (see below) | | `-Wno-deprecated-volatile` | The detection code for C++20 in the CI image (based on Ubuntu `focal`) relies on deprecated volatiles (see below) | | `-Wno-ambiguous-reversed-operator` | Due to an ambiguous operator in a Dash unit test ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/test/fuzz/addrman.cpp#L145)) (see below) | | `-Wno-deprecated-enum-enum-conversion` | Due to key combination code in Dash Qt ([source](https://github.com/dashpay/dash/blob/2dacfb08bdf02fdaf0740edac19435bfaa0e52d3/src/qt/bitcoingui.cpp#L376)) (see below) | <details> <summary>-Wno-deprecated-builtins</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' CXX bench/bench_dash-addrman.o In file included from bench/addrman.cpp:6: In file included from ./bench/bench.h:16: ./bench/nanobench.h:952:13: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins] return !ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(Decayed) || sizeof(Decayed) > sizeof(long) || std::is_pointer<Decayed>::value; ^ ./bench/nanobench.h:107:57: note: expanded from macro 'ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE' # define ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__) ^ 1 error generated. make[2]: *** [Makefile:14567: bench/bench_dash-addrman.o] Error 1 make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> <details> <summary>-Wno-deprecated-volatile</summary> ``` > ./configure [...] [...] checking whether clang++-16 supports C++20 features with -h std=c++20... no checking whether clang++-16 supports C++20 features with -std=c++2a... no checking whether clang++-16 supports C++20 features with +std=c++2a... no checking whether clang++-16 supports C++20 features with -h std=c++2a... no configure: error: *** A compiler with support for C++20 language features is required. > cat config.log This file contains any messages produced by compilers while running configure, to aid debugging if configure makes a mistake. It was created by Dash Core configure 21.0.0, which was generated by GNU Autoconf 2.69. Invocation command line was $ ./configure --prefix=/src/dash/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --disable-crash-hooks --disable-stacktraces --enable-c++20 --enable-suppress-external-warnings --enable-debug --disable-ccache --with-sanitizers=thread [...] configure:4450: checking whether clang++-16 accepts -g configure:4470: clang++-16 -c -g -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5 configure:4470: $? = 0 configure:4511: result: yes configure:4537: checking whether make supports the include directive configure:4552: make -f confmf.GNU && cat confinc.out this is the am__doit target configure:4555: $? = 0 configure:4574: result: yes (GNU style) configure:4599: checking dependency style of clang++-16 configure:4710: result: gcc3 configure:4756: checking whether clang++-16 supports C++20 features with -std=c++20 configure:5558: clang++-16 -std=c++20 -c -pipe -O2 -Werror -Werror=reorder -Werror=thread-safety -Wno-deprecated-builtins -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5 conftest.cpp:109:36: error: volatile-qualified parameter type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile] test(const int c, volatile int v) // 'volatile is deprecated in C++20' ^ 1 error generated. ``` </details> <details> <summary>-Wno-ambiguous-reversed-operator</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' [...] CXX test/fuzz/fuzz-addrman.o test/fuzz/addrman.cpp:329:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'CAddrManDeterministic' and 'CAddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator] assert(addr_man1 == addr_man2); ~~~~~~~~~ ^ ~~~~~~~~~ /usr/include/assert.h:93:27: note: expanded from macro 'assert' (static_cast <bool> (expr) \ ^~~~ test/fuzz/addrman.cpp:145:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed bool operator==(const CAddrManDeterministic& other) ^ test/fuzz/addrman.cpp:145:10: note: mark 'operator==' as const or add a matching 'operator!=' to resolve the ambiguity 1 error generated. make[2]: *** [Makefile:15393: test/fuzz/fuzz-addrman.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[3]: Leaving directory '/src/dash/src/dashbls' make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> <details> <summary>-Wno-deprecated-enum-enum-conversion</summary> ``` > make Making all in src make[1]: Entering directory '/src/dash/src' make[2]: Entering directory '/src/dash/src' make[3]: Entering directory '/src/dash' make[3]: Leaving directory '/src/dash' [...] CXX qt/libbitcoinqt_a-bitcoingui.o qt/bitcoingui.cpp:376:51: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion] quitAction->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_Q)); ~~~~~~~~ ^ ~~~~~~~~~ qt/bitcoingui.cpp:609:56: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion] minimize_action->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_M)); ~~~~~~~~ ^ ~~~~~~~~~ 2 errors generated. make[2]: *** [Makefile:12803: qt/libbitcoinqt_a-bitcoingui.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:18568: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:800: all-recursive] Error 1 ``` </details> This pull request removes the need for the last two `-Wno*` flags, removes some leftover `boost::`{`function`, `lexical_cast`} usage and some `gArgs` usage where a local variable would be more applicable. Additionally, in some builds, there is an deprecation warning (`-Wdeprecation`) for using `[=]` and relying on its implicit capture of `this` (see below), this can be seen during GCC builds but not Clang builds and correspond to a proposal to the C++20 standard ([proposal](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html)). It has also been mentioned at https://en.cppreference.com/w/cpp/language/lambda, "The implicit capture of `*this` when the capture default is `=` is deprecated. _(since C++20)_". This has also been remedied as part of this PR. <details> <summary>-Wdeprecated</summary> ``` > make -j10 [...] CXX qt/libbitcoinqt_a-trafficgraphwidget.o qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:195:63: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 195 | connect(appearance, &AppearanceWidget::appearanceChanged, [=](){ | ^ qt/optionsdialog.cpp:195:63: note: add explicit 'this' or '*this' capture qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:277:55: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 277 | connect(ui->coinJoinEnabled, &QCheckBox::clicked, [=](bool fChecked) { | ^ qt/optionsdialog.cpp:277:55: note: add explicit 'this' or '*this' capture qt/optionsdialog.cpp: In lambda function: qt/optionsdialog.cpp:293:45: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated] 293 | connect(this, &OptionsDialog::rejected, [=]() { | ^ qt/optionsdialog.cpp:293:45: note: add explicit 'this' or '*this' capture CXX qt/libbitcoinqt_a-utilitydialog.o CXX qt/libbitcoinqt_a-addressbookpage.o [...] ``` </details> ## 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 **(note: N/A)** - [x] 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: PastaPastaPasta: utACK 740d25c knst: utACK 740d25c Tree-SHA512: 52793f9862192ccb556debcac2ec766c4f86e69ec4bedc2fdbaaff965ab4ac478b65b3983b1cb3a92610db85aee6f8668b0ded3494c617c5af7d8801284e461b
fad6269 test: Assert that exit code indicates failure (MarcoFalke) faecb72 test: Fix intermittent issue in p2p_segwit.py (MarcoFalke) Pull request description: Calling `start_node` might call `wait_for_rpc_connection`, which will fail. https://cirrus-ci.com/task/5669555591708672?logs=ci#L3504 ``` File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_segwit.py", line 1974, in test_upgrade_after_activation self.start_node(2, extra_args=["-reindex", f"-segwitheight={SEGWIT_HEIGHT}"]) File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 508, in start_node node.wait_for_rpc_connection() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection raise FailedToStartError(self._node_msg( test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status 1 during initialization ACKs for top commit: jnewbery: ACK fad6269 dhruv: ACK fad6269 Tree-SHA512: 4c5e39ce25e135717ea433258518f93f09d1c528c4538a8627d3da13bc0c0ba4b45911703c26392ff0f5e0cb7831a6c7cc53e6e29102d3da9c8cfce7cef333cc
…eCoinControlState 5f438d6 refactor, qt: Simplify SendCoinsDialog::updateCoinControlState (João Barbosa) Pull request description: This PR doesn't change behaviour, removes the coin control argument from `updateCoinControlState` since it's a class member. ACKs for top commit: hebasto: ACK 5f438d6, I have reviewed the code and it looks OK, I agree it can be merged. jonatack: Code review ACK 5f438d6 kristapsk: utACK 5f438d6. Code looks correct. Tree-SHA512: 14abaa3d561f8c8854fed989b6aca886dcca42135880bac76070043f61c0042ec8967f2b83e50bbbb82050ef0f074209e97fa300cb4dc51ee182316e0846506d
c5a470e refactor: Drop CCoinControl::SetNull (João Barbosa) Pull request description: The only external call to `SetNull` is changed as follow ```diff - m_coin_control->SetNull(); + m_coin_control = std::make_unique<CCoinControl>(); ``` ACKs for top commit: theStack: Code-Review ACK c5a470e MarcoFalke: review ACK c5a470e 🍤 Tree-SHA512: 2588828720cdcf405fc4fb06f2aa17ad4eef2a645e12d820311006127e732258dd084993f17f23742f8e7f782e18289a6199dcec3690efc9b92458f90b816a8f
fab1eb6 test: Fix feature_config_args.py intermittent issue (MarcoFalke) Pull request description: Fix bitcoin#21448 ACKs for top commit: laanwj: Code review ACK fab1eb6 Tree-SHA512: cad9f684f43aa801d0c1cb5f1684ffa624df1216be225cea46b1389ba2b67cbd6159ffb786fe144bf1ca865623dd9a10289d4293cfabb678bdb243d4ea00734d
…d nChainTx 91d93aa validation: remove nchaintx from assumeutxo metadata (James O'Beirne) 931684b validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne) Pull request description: This fixes an oversight from the move of nChainTx from the user-supplied snapshot metadata into the hardcoded assumeutxo chainparams. Since the nChainTx is now unused in the metadata, it should be removed in a future commit. See: bitcoin#19806 (comment) ACKs for top commit: Sjors: utACK 91d93aa ryanofsky: Code review ACK 91d93aa. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests. Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
fa5cb6b fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke) Pull request description: This is needed when stdout is polluted by the fuzz engine. stderr can't be used instead because it is polluted by aborting the program. Top commit has no ACKs. Tree-SHA512: bf0a2a6bcd964ff1f0f3ef6e7e297b4c780430c4d6312332ed99ace0e1c58243c1483fd387e39405837d39b36072dfeb9ae03d2a7aa728ad6955159754fd5766
…l option fa27d6d fuzz: Remove unused --enable-danger-fuzz-link-all option (MarcoFalke) Pull request description: Remove the unused build option, which was *dangerous* (as the name implies). Also remove the fuzzbuzz config, which was never used as part of this repo and seems redundant now that we integrate with oss-fuzz. ACKs for top commit: practicalswift: cr ACK fa27d6d: patch looks correct and rationale makes sense hebasto: ACK fa27d6d, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 9bd65ed6a76d13d8d9c7a88aaae30f701215d5d0619693a3115d5ec350808aaf6a1aa4737466a5b96f3948513ec4d063808fe16219818366720e247880a15177
08f3dbb test: Bump shellcheck version (Hennadii Stepanov) Pull request description: The changelog for v0.7.2 is available [here](https://github.com/koalaman/shellcheck/blob/v0.7.2/CHANGELOG.md). Only [SC2268](https://github.com/koalaman/shellcheck/wiki/SC2268) requires to update our code. ACKs for top commit: jarolrod: ACK 08f3dbb Tree-SHA512: 4585cd1f4d9def2fbaafe5a2a57761288d432781eb8c6c6d37064727d7ca8fc3f35c552e6a2ffdf0820d753d4bde2c8e43e5f3f57d242f5f57591a9b1b03558d
ffe33df chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns) f054f6b versionbits: simplify state transitions (Anthony Towns) 55ac5f5 versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns) dd07e6d fuzz: test versionbits delayed activation (Anthony Towns) dd85d54 tests: test versionbits delayed activation (Anthony Towns) 73d4a70 versionbits: Add support for delayed activation (Anthony Towns) 9e6b65f tests: clean up versionbits test (Anthony Towns) 5932744 tests: test ComputeBlockVersion for all deployments (Anthony Towns) 63879f0 tests: pull ComputeBlockVersion test into its own function (Anthony Towns) Pull request description: BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html Edge cases are tested by fuzzing added in bitcoin#21380. ACKs for top commit: instagibbs: tACK bitcoin@ffe33df jnewbery: utACK ffe33df MarcoFalke: review ACK ffe33df 💈 achow101: re-ACK ffe33df gmaxwell: ACK ffe33df benthecarman: ACK ffe33df Sjors: ACK ffe33df jonatack: Initial approach ACK ffe33df after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore. ariard: Code Review ACK ffe33df Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
This command line argument is substitute for -segwitheight so far as dash does not have a segwit feature
… to fix timeouts, speed up ccd976d test: use 327 fewer blocks in feature_nulldummy (Jon Atack) 68c280f test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack) Pull request description: The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug` Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion. ACKs for top commit: AnthonyRonning: reACK ccd976d - ran a few times with the rest of the tests and still passing for me with just the fewer block change. MarcoFalke: review ACK ccd976d 🏝 Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
ForEachNode is publicly available, which will help us extract the functions from CConnman in a subsequent commit
Required to avoid crashes when calling RelayInvFiltered in situations
where the PeerManager* atomic hasn't been set (possible when ProcessMessage
isn't called, leaving the value unset, while a separate thread traverses
the ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] >
ProcessInstantSendLock > RelayInvFiltered call chain).
[1] - There is a function with the exact same name but with multiple
arguments
…use `git diff-range` f5f8bd9 doc: replace gfd() function to recommendation to use `git diff-range` (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented `gfd` is less convenient to use, `git range-diff` is more functional. Dropped recommendation to use it. ## What was done? Updated `CONTRIBUTION.md` file ## How Has This Been Tested? Run `git range-diff` to ensure that it indeed works ## 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 - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 08663e5371753b6851b216da66ef72bb6bfb80acd42c31c57de9f8d52f00766e3121adf960e27236451d6520088f0c2c927f5f4f3001a40c591feb62074951e0
…erManager, remove CConnman::RelayTransaction f2fe39f trivial: add `Asserts` for `m_peerman` pointer container uses (Kittywhiskers Van Gogh) 35be4e2 llmq: pass PeerManager to llmq::CInstantSendManager constructor (Kittywhiskers Van Gogh) bfd33cd net: move CConnman::RelayInv{Filtered} into PeerManager (Kittywhiskers Van Gogh) 313a7e9 trivial: cleanup unnecessary headers in context files (Kittywhiskers Van Gogh) c3f1ac2 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction (Kittywhiskers Van Gogh) 0323c6c net: move Relay{Inv, InvFiltered, Transaction} out of CConnman (Kittywhiskers Van Gogh) d54ba44 net: use ForEachNode instead of manually iterating through vNodes (Kittywhiskers Van Gogh) Pull request description: ## Additional information * Dependency for dashpay#5982 * Unfortunately, we are stuck with using the `unique_ptr` const-ref of `PeerManager` for initializing {`CJ`, `LLMQ`}`Context` due to some unit tests depending on Dash-specific entities but based on a testing setup that doesn't initialize `PeerManager` (specifically `validation_chainstatemanager_tests`, which uses `ChainTestingSetup`, as opposed to `TestingSetup`, which initializes `PeerManager`). Attempting to dereference it earlier will result in crashing (see CI pipeline [here](https://gitlab.com/dashpay/dash/-/pipelines/1245924304)) ## Breaking Changes None observed. ## 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 [f2fe39f](dashpay@f2fe39f) knst: utACK [f2fe39f](dashpay@f2fe39f) Tree-SHA512: 5e9ca47ba9f7f0c5fdc93bf6cfd99d91be6d134610d2bd3dccde17443a956c7d8848b3406082ee8b6ee3fd25f586ce7bac45bc89b568424479b8fc149172ae51
…1681, bitcoin#21714, bitcoin#21749, bitcoin#21792, bitcoin#21814, bitcoin#21874, bitcoin#21884, bitcoin-core/gui#284 7cc77f3 Merge bitcoin#21373: test: generate fewer blocks in feature_nulldummy to fix timeouts, speed up (MarcoFalke) a933a60 feat: new command line argument -bip147height for bitcoin#21373 (Konstantin Akimov) 5191138 Merge bitcoin#21377: Speedy trial support for versionbits (fanquake) ecade9b Merge bitcoin#21749: test: Bump shellcheck version (W. J. van der Laan) eeec2f2 Merge bitcoin#21884: fuzz: Remove unused --enable-danger-fuzz-link-all option (fanquake) 51633d7 Merge bitcoin#21874: fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke) a02a2c0 Merge bitcoin#21681: validation: fix ActivateSnapshot to use hardcoded nChainTx (MarcoFalke) 71f23d6 Merge bitcoin#21814: test: Fix feature_config_args.py intermittent issue (MarcoFalke) de4d2a8 Merge bitcoin#21714: refactor: Drop CCoinControl::SetNull (MarcoFalke) a63f9c3 Merge bitcoin-core/gui#284: refactor: Simplify SendCoinsDialog::updateCoinControlState (Hennadii Stepanov) b2d8893 Merge bitcoin#21792: test: Fix intermittent issue in p2p_segwit.py (MarcoFalke) Pull request description: ## Issue being fixed or feature implemented Regular batch of backports from bitcoin v22 ## What was done? Implemented new commandline argument `-bip147height` for RegTest. Backports: - bitcoin#21377 - bitcoin#21792 - bitcoin-core/gui#284 - bitcoin#21714 - bitcoin#21373 - bitcoin#21814 - bitcoin#21681 - bitcoin#21874 - bitcoin#21884 - bitcoin#21749 ## How Has This Been Tested? Run unit/functional tests ## 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 ACKs for top commit: PastaPastaPasta: utACK 7cc77f3 Tree-SHA512: d46218667430af19c445d67d6411b1e7f19920e85f86e6e74ae7b9062aeb3763637a8613587c203ba8d285ccc3ee755f936141010944cfae8627397e8b8584d3
9096b13 net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov) Pull request description: It is not possible to have a node in `CConnman::vNodesDisconnected` and its reference count to be incremented - all `CNode::AddRef()` are done either before the node is added to `CConnman::vNodes` or while holding `CConnman::cs_vNodes` and the object being in `CConnman::vNodes`. So, the object being in `CConnman::vNodesDisconnected` and its reference count being zero means that it is not and will not start to be used by other threads. So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will always succeed and is not necessary. Indeed all locks of `CNode::cs_vSend` are done either when the reference count is >0 or under the protection of `CConnman::cs_vNodes` and the node being in `CConnman::vNodes`. ACKs for top commit: MarcoFalke: review ACK 9096b13 🏧 jnewbery: utACK 9096b13 Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
fa09a9e style: Add { } to multi-line if (MarcoFalke) fadafab move-only: Move functions to blockstorage (MarcoFalke) fa7e64d move-only: Move constants to blockstorage (MarcoFalke) fa247a3 refactor: Move block storage globals to blockstorage (MarcoFalke) fa81c30 refactor: Move pruning/reindex/importing globals to blockstorage (MarcoFalke) Pull request description: See bitcoin#21575 ACKs for top commit: Sjors: ACK fa09a9e kiminuo: ACK fa09a9e laanwj: Code review ACK fa09a9e promag: Code review ACK fa09a9e. Since last review Tree-SHA512: 2eb6962ff44da6b77f3058fc02ec66ab742e25ae8dcc8ec62b062896571910d43ca7c4bb16fb3ccb5e5245195b8dec6384b6c8d442fa97ca28d93bdff347d677
47c3ea0 doc: add OSS-Fuzz section to fuzzing.md doc (Adam Jonas) Pull request description: This adds documentation about [Bitcoin Core's participation](https://github.com/google/oss-fuzz/pull/5699/files) in Google's OSS-Fuzz program and adds the caveat that the project may not disclose vulnerabilities within the 90-day window described in the [program's disclosure guidelines](https://google.github.io/oss-fuzz/getting-started/bug-disclosure-guidelines/). ACKs for top commit: jonatack: ACK 47c3ea0 Tree-SHA512: 87bf0146fb74d1e4b3b8839e6c8f3d53046008a6d5b926ffe5b95be3c396a5e47e47967533422f60b04c4446482f49d210ada410b742f69781a7afde623d704d
7031721 rpc/listaddressgroupings: redefine inner-most array as ARR_FIXED (Karl-Johan Alm) 8500f7b rpc/createrawtransaction: redefine addresses as OBJ_USER_KEYS (Karl-Johan Alm) d9e2183 rpc: include OBJ_USER_KEY in RPCArg constructor checks (Karl-Johan Alm) Pull request description: This PR adjusts the two issues I encountered while developing a tool that converts RPCHelpMan objects into bindings for other language(s). The first is in createrawtransaction, where the address part, e.g. bc1qabc in > createrawtransaction '[]' '[{"bc1qabc": 1.0}]' is declared as a `Type::OBJ`, when in reality it should be a `Type::OBJ_USER_KEYS`, defined as such: https://github.com/bitcoin/bitcoin/blob/5925f1e652768a9502831b9ccf78d16cf3c37d29/src/rpc/util.h#L126 (coincidentally, this is the first and only (afaict) usage of this `RPCArg::Type`). The second is in the `listaddressgroupings` RPC, which returns an array of arrays of arrays, where the innermost one is a tuple-thingie with an optional 3rd item; this is an `ARR_FIXED`, not an `ARR`. ACKs for top commit: MarcoFalke: ACK 7031721 🐀 Tree-SHA512: 769377416c6226d1738a956fb685498e009f9e7eb2d45bc679b81c5364b9520fdbcb49392c937ab45598aa0d33589e8e6a59ccc101cf8d8e7dfdafd58d4eefd0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
bitcoin backports