8858e88c840197cdcabea07dd1380ef2aa4ece02 p2p: refactor: tidy up `PeerManagerImpl::Misbehaving(...)` (Sebastian Falbesoner)
Pull request description:
This simple refactoring PR has the goal to improve the readability of the `Misbehaving` method by
- introducing constant variables `score_before` and `score_now` (to avoid repeatedly calculating the former)
- deduplicating calls to LogPrint(), eliminates else-branch
ACKs for top commit:
jnewbery:
utACK 8858e88c840197cdcabea07dd1380ef2aa4ece02
rajarshimaitra:
tACK 8858e88c84
Tree-SHA512: 1d4dd5ac1d16ee9595edf4fa46e4960915a203641d74e6c33cffaba62ea71328834309a4451256fb45daf759f0cf6f4f199c46815afff6c89c0746e2ad4d4092
fa108d6a757838225179a8df942cfb6d99c98c90 test: update tests for peer discouragement (Jon Atack)
1a9f462caa63fa16d7b4415312d2032a42b3fe0b gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
laanwj:
ACK fa108d6a757838225179a8df942cfb6d99c98c90
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
41d55d30579358c805036201664ad6a1c1d48681 doc: getpeerinfo banscore deprecation release note (Jon Atack)
dd54e3796e633cfdf6954af306afd26eadc25116 test: getpeerinfo banscore deprecation test (Jon Atack)
8c7647b3fbbab03ea84071cf3cd2d0d2bf8be255 rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack)
Pull request description:
Per https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592, this PR deprecates returning the `banscore` field in the `getpeerinfo` RPC, updates the help, adds a test, and updates the release notes. Related to #19464.
ACKs for top commit:
fanquake:
ACK 41d55d30579358c805036201664ad6a1c1d48681
Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc
0e51a355128a825d428fe2b9017c25085731fc04 refactor: Use Mutex type for some mutexes in CNode class (Hennadii Stepanov)
Pull request description:
No need the `RecursiveMutex` type for the `CNode::cs_vSend`, `CNode::cs_hSocket` and `CNode::cs_vRecv`.
Related to #19303.
ACKs for top commit:
jnewbery:
utACK 0e51a35512
MarcoFalke:
review ACK 0e51a355128a825d428fe2b9017c25085731fc04 🔊
Tree-SHA512: 678ee5e3c15ad21a41cb86ec7179741bd505a138638fdc07f41d6d677c38fbf2208219bfc0509e3675e721fc8d8816e858070db7b87c5d72ad93aae81f7e1636
57b6fe7327 Merge bitcoin/bitcoin#22257: test: refactor: various (de)serialization helpers cleanups/improvements (MarcoFalke)
5c85b7dc14 Merge bitcoin/bitcoin#21056: rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (W. J. van der Laan)
eaa5f8ea31 Merge bitcoin/bitcoin#20772: fuzz: bolster ExtractDestination(s) checks (MarcoFalke)
17598db793 Merge bitcoin/bitcoin#22408: test: add tests for `bad-txns-prevout-null` reject reason (MarcoFalke)
Pull request description:
This PR and does not introduce breaking changes
## Issue being fixed or feature implemented
Bitcoin backports
## What was done?
backports bitcoin PRS 22408 ,20772, 21056, 22257, 19776, 20599
## How Has This Been Tested?
In CI
## 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
Top commit has no ACKs.
Tree-SHA512: 7261331876606bd46725116ecfcc39765428727840ecd557eee59ebd04de535485e1b02f15d03c5b8d58553bf1a62378707dd5eda08df9f1f9d0bf7171020815
bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
191405420815d49ab50184513717a303fc2744d6 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f8f81c78cf84117a87074c6ff6c9d95 test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958c4a10ba20dc86c011d71cda4b070a5 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
b9e76f1bf08c52fcd402b2314e00db4ad247ebc8 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7dc9a7b0c55d28011161606399417664 rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef698a1695050f82ef6567b3b87a21861 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59fe51473571661316214156fbdbdcae rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8
promag:
ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1f449586a9e39bc4fb53cb5c7a31362e47aea19b test: add `bad-txns-prevout-null` test to mempool_accept.py (Sebastian Falbesoner)
aa0a5bb70d77739d43d5a9ceae78fb0c6fafd435 test: add `bad-txns-prevout-null` test case to invalid_txs.py (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing tests for the reject reason `bad-txns-prevout-null`, which is thrown in the function `CheckTransaction()`: a62fc35a15/src/consensus/tx_check.cpp (L52-L54)
Basically this condition is met for non-coinbase transactions (the code snippet above only hits if `!tx.IsCoinBase()`) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff.
Can be tested by running the functional tests `feature_block.py`, `p2p_invalid_tx.py` and `mempool_accept.py`. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?).
ACKs for top commit:
rajarshimaitra:
tACK 1f449586a9
brunoerg:
tACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b
kristapsk:
ACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b, code looks correct and all tests pass.
Tree-SHA512: 2d4f940a6ac8e0d80d2670c9e1111cbf43ae6ac62809a2ccf17cffee9a41d387ea4d889ee300eb4a407c055b13bfa5d37102a32ed59964a9b6950bd907ba7204
fa6847d2fb refactor: drop circular dependencies over deterministicmns in validationinterface (Konstantin Akimov)
f8d1853903 refactor: even more passing CDeterministicMNManager by ref (Kittywhiskers Van Gogh)
d4aa891735 refactor: more passing CDeterministicMNManager by ref (Kittywhiskers Van Gogh)
e628d7517a refactor: pass CDeterministicMNManager by ref to CJContext members (Kittywhiskers Van Gogh)
d731f4127e refactor: pass CDeterministicMNManager by ref to LLMQContext members (Kittywhiskers Van Gogh)
6bd23f40aa refactor: pass CDeterministicMNManager by ref to CGovernanceManager (Kittywhiskers Van Gogh)
055dbba1fa refactor: move GetListAtChainTip() calls out of llmq::utils::*, misc changes (Kittywhiskers Van Gogh)
da39b73f01 refactor: move GetListAtChainTip() calls out of CGovernanceVote (Kittywhiskers Van Gogh)
e9de972982 refactor: move GetListAtChainTip() calls out of CGovernanceObject (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/5929
## Breaking Changes
None. Changes are limited to refactoring, no logical changes have been made.
## 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:
(self) utACK fa6847d2fb
knst:
utACK fa6847d2fb
Tree-SHA512: f1acf44bad25789d03c48788a3ac1807d7c553ee45164276ffa6f0dc39dc02e5d29d364469633aa84b341e8c4c13508e0e7a114de6e01fb624f256acfd97199b
140e91fdca merge bitcoin#20864: Move SocketSendData lock annotation to header (Kittywhiskers Van Gogh)
e432122cbd net: Move CConnman/NetEventsInterface after CNode in header file (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Broken off from upcoming backport PR relating to networking due to primarily move-only large diff.
Review large-diff commit using `git diff develop..e432122cbd1cc9bec45cc01c3eb4194a05af1d0e --color-moved=dimmed-zebra --patience` ([source](fa0a71781a))
## Breaking Changes
None. Changes are primarily move-only with minor 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 **(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 **(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 140e91fdca
Tree-SHA512: f2a42532819085aa8d0908b7b102f8d777681d247cd5cecf7f46f71435df2408f39fddd52382ed55ccee686a185092b0dd7f5029b2610be56d3a57b309466238
5a6b8b6b1f partial Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX with avoidpartialspends (fanquake)
2f788aa76d fix: change port to use for zmq in interface_zmq_dash.py (Konstantin Akimov)
0ce66fd477 Merge #19507: Expand functional zmq transaction tests (Wladimir J. van der Laan)
a1c2386153 Merge #17445: zmq: Fix due to invalid argument and multiple notifiers (Wladimir J. van der Laan)
44929bad82 Merge #16404: qa: Test ZMQ notification after chain reorg (MarcoFalke)
1707f01309 fix: follow-up changes from bitcoin/bitcoin#22220 for maxapsfee (Konstantin Akimov)
eb4270deae Merge #19743: -maxapsfee follow-up (Samuel Dobson)
6a6d379711 Merge #19756: tests: add sync_all to fix race condition in wallet groups test (MarcoFalke)
5821a1d23a Merge #14582: wallet: always do avoid partial spends if fees are within a specified range (Samuel Dobson)
59d5a4ef39 Merge #19773: wallet: Avoid recursive lock in IsTrusted (Samuel Dobson)
2489f29f0e Merge #19830: test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles (fanquake)
10fa7a66b6 Merge #19538: ci: Add tsan suppression for race in DatabaseBatch (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19538
- bitcoin/bitcoin#19830
- bitcoin/bitcoin#19773
- bitcoin/bitcoin#14582
- bitcoin/bitcoin#19756
- bitcoin/bitcoin#19743
- bitcoin/bitcoin#16404
- bitcoin/bitcoin#17445
- bitcoin/bitcoin#19507
- partial bitcoin/bitcoin#27053
+some extra fixes and missing changes from bitcoin/bitcoin#22220 for `maxapsfee`
+changed port for zmq in `interface_zmq_dash.py` to prevent intermittent error in `interface_zmq.py`
## How Has This Been Tested?
Run unit & functional tests
## Breaking Changes
`CreateTransaction` now uses sometime 2 private keys for one transaction instead one
## 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 5a6b8b6b1f
Tree-SHA512: 7efd8a31808f155c08035d0fb7ceaac369e3e44e68d2c91a88e52815a60efba5fe9458f41d93352627c2c062d414fb0207dcf216fa75b54af210b503f9123de6
991c9ec1a2 refactor: accept NodeContext arg into BlockAssembler instead of managers (Kittywhiskers Van Gogh)
c427305805 refactor: remove CNetFulfilledRequestManager global, move to NodeContext (Kittywhiskers Van Gogh)
a53046c4a9 refactor: remove CDSTXManager global and alias, move to CJContext (Kittywhiskers Van Gogh)
f0451fb98d refactor: remove CCreditPoolManager global, move to NodeContext (Kittywhiskers Van Gogh)
67cfee70f8 refactor: remove CMNHFManager::GetSignalsStage alias from CChainState (Kittywhiskers Van Gogh)
667852c851 refactor: cleanup CDSNotificationInterface member names, add asserts (Kittywhiskers Van Gogh)
0d6f736a19 fix: add missing entity for destruction in DashTestSetupClose (Kittywhiskers Van Gogh)
2dd6082e54 refactor: move special transaction processing into helper class (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/5908
## Breaking Changes
None. Changes are limited to refactoring, no logical changes have been made.
## 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 991c9ec1a2
Tree-SHA512: 7a8d90416b6de4915639908b5f6bd59caee73e26462864d12a14a5d74af300a3733d8d9b0863e267b82d2807a8a5753acf5574306a0493bb7f3d83d8f0a7da4f
c9ffb72fb5 fix: avoid `hSocket` double lock (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
It's is locked in `CloseSocketDisconnect()` already.
To be merged before #5511 or 19915 backport is going to cause issues otherwise.
## What was done?
Assert the lock is held already, instead of locking it again.
## How Has This Been Tested?
Run tests, run a node on testnet and drop connections to peers
## 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)_
ACKs for top commit:
PastaPastaPasta:
utACK c9ffb72fb5
Tree-SHA512: 6042d0683bf9b1326a74d73b5b44896a0470865b88e1c686d6eefe55c0d419b11a134474499bb6c9c308af69b2f7c4a60070d4535072304330cb640f91f5e367
Decoupling initialization from loading the database means we need to
assert if the database is actually loaded before performing any operations
on it (i.e. check if the manager is "valid")
c5031685bc fix: rename arguments for 'voteraw' (Konstantin Akimov)
3621966f12 feat: add todo to drop Throw() from rpc/util.h (Konstantin Akimov)
b54f03a0c1 fix: wrong name of argument for coinjoin (Konstantin Akimov)
d0163543d9 refactor: use new format CPCCommand for rpc/coinjoin (Konstantin Akimov)
0e1a31159f Merge #19994: Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke)
af9eb81e56 fix: wrong name of arguments for RPC (Konstantin Akimov)
c30c8f22dd Merge #19849: Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) (MarcoFalke)
f525f574b0 fix: follow-up missing changes from Merge #18607: rpc: Fix named arguments in documentation (Konstantin Akimov)
7ac1ee0fb4 Merge #19717: rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump) (MarcoFalke)
860d31f504 Merge #19455: rpc generate: print useful help and error message (Wladimir J. van der Laan)
41c35fd8dc fix: adjust missing arguments and help for misc rpc: debug, echo, mnsync (Konstantin Akimov)
58d923cd5b Merge #19528: rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
This batch of backports asserts that RPCArg names are equal to CRPCCommand ones.
## What was done?
done backports:
- bitcoin/bitcoin#19994
- bitcoin/bitcoin#19849
- bitcoin/bitcoin#19717
- bitcoin/bitcoin#19455
- bitcoin/bitcoin#19528
Beside that same changes are applied for src/coinjoin's rpc.
There's also applied multiple fixes for various rpcs for cases when RPCArg names are mismatched with CPCCommand
**Please, note, that this PR is not final fix for all RPCArgs**. There's a lot of dash's rpc that is not refactored that.
That it is not easy to implement for `quorum command` because the list of arguments (and even their numbers) are different for each sub-command. This fixes are out-of scope of this PR and should be done before bitcoin#18531 is backported.
See also relevant bitcoin#21035.
## How Has This Been Tested?
I used this helper to see which exactly args are specified wrongly:
```cpp
diff --git a/src/rpc/server.h b/src/rpc/server.h
index d4a7ba60eb..cdfd741f54 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -16,6 +16,7 @@
#include <string>
#include <univalue.h>
+#include <logging.h>
class CRPCCommand;
@@ -110,6 +111,19 @@ public:
fn().GetArgNames(),
intptr_t(fn))
{
+ if (fn().m_name != name_in || fn().GetArgNames() != args_in) {
+ std::cerr << "names: " << fn().m_name << ' ' << name_in << std::endl;
+ std::cerr << "arg names: " << fn().GetArgNames().size() << std::endl;
+ for (const auto& i : fn().GetArgNames()) {
+ std::cerr << "arg: " << i << std::endl;
+ }
+ std::cerr << "FIASCO FIASCO FIASCO FIASCO FIASCO FIASCO" << std::endl;
+ }
CHECK_NONFATAL(fn().m_name == name_in);
CHECK_NONFATAL(fn().GetArgNames() == args_in);
}
```
## Breaking Changes
N/A
Some arguments are renamed in RPC but they have been broken (used incorrect name not same as in docs)
## 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:
(self)utACK c5031685bc
Tree-SHA512: e885a8f8fa8bc282dae092fe8df65a37e2ab6ca559cd0598d54bfc06cacddb3bd6f3c74fa2d9c1551f8a4fbdfdeabb8d065649df66d5809e792aec6f51d0df14
86dd0eabb4 chore: increase amount of build jobs from 4 to 8 for depends (Konstantin Akimov)
Pull request description:
## What was done?
Bump CI yaml to use 8 threads for build depends
## How Has This Been Tested?
Builds logs says:
```
$ make -j$(nproc) -C depends HOST=$HOST $DEP_OPTS
make: Entering directory '/builds/dashpay/dash/depends'
```
https://gitlab.com/dashpay/dash/-/jobs/6409522172
## 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 86dd0eabb4
Tree-SHA512: dfdcae5473a5315d3fc64721f432d671a14cab9811305334f948a73ddc13e5f3e58be433988030ef6019cddd3f383c87722eaed40d8b3ab570e5c224c20191e0
dfddfd09a4 trivial: remove unneeded header, enumerate circular dependencies (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
As found in https://github.com/dashpay/dash/pull/5929#discussion_r1526663050, `validation.h` is not needed for `specialtxman.cpp` and removing the header uncovers other circular dependencies that were obscured by the shortest circular path (see https://github.com/dashpay/dash/pull/5929#discussion_r1527594636).
## Breaking Changes
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 **(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 dfddfd09a4
Tree-SHA512: c3e74c1a381c0b69985a29f57da3565ec7681548389f09ab8b9907386f0a9f221db92a39409eb46595f3546edffa389774b96b0c9dd8f4fedff46f2d3bd635f1
14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
7356292e1d7a44da8a2bd31c02c58d550bf38009 Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983e57cc97ecbc56d0177eaf1854c842c Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600ca9c4b676fa2f97520b8ecc44642246 Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c69bc85e96b08ade725594eda3e230f Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)
Pull request description:
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion:
1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)
ACKs for top commit:
laanwj:
code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009
promag:
Code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009.
Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
3e730bf90aaf53c41ff3a778f6aac15d163d1c0c zmq: Fix due to invalid argument and multiple notifiers (João Barbosa)
Pull request description:
ZMQ initialization is interrupted if any notifier fails, and in that case all notifiers are destroyed. The notifier shutdown assumes that the initialization had occurred. This is not valid when there are multiple notifiers and any except the last fails to initialize.
Can be tested by running test/functional/interface_zmq.py from this branch with bitcoind from master.
Closes#17185.
ACKs for top commit:
laanwj:
Code review ACK 3e730bf90aaf53c41ff3a778f6aac15d163d1c0c, thanks for adding a test
Tree-SHA512: 5da710e97dcbaa94896d019e75162d470f6d381ee07c60e5b3e9db93d11e8f7ca9bf2c509efa4486199e88c96c3e720cc96b4e35b62725d4c7db8e8e9bf6e09d
72ae20fc142457a200278cb2fedc5e32a3766b58 tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm)
Pull request description:
This most likely fixes#19749, the intermittent CI issues with wallet_groups.
This fix is also included in #19743.
Top commit has no ACKs.
Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f
