76a458e5f9 fmt: apply formatting suggestions from `clang-format-diff.py` (Kittywhiskers Van Gogh)
63962ec475 merge bitcoin#28165: transport abstraction (Kittywhiskers Van Gogh)
c6b9186e69 merge bitcoin#25325: Add pool based memory resource (Kittywhiskers Van Gogh)
8c986d6b08 partial bitcoin#27981: Fix potential network stalling bug (Kittywhiskers Van Gogh)
13f6dc1b27 merge bitcoin#26844: Pass MSG_MORE flag when sending non-final network messages (Kittywhiskers Van Gogh)
caaa0fda01 net: use `std::deque` for `vSendMsg` instead of `std::list` (Kittywhiskers Van Gogh)
2ecba6ba5f partial bitcoin#26036: add NetEventsInterface::g_msgproc_mutex (Kittywhiskers Van Gogh)
f6c943922f merge bitcoin#24543: Move remaining globals into PeerManagerImpl (Kittywhiskers Van Gogh)
dbe41ea141 refactor: move object request logic to `PeerManagerImpl` (Kittywhiskers Van Gogh)
112c4e0a16 merge bitcoin#24021: Rename and move PoissonNextSend functions (Kittywhiskers Van Gogh)
6d690ede82 merge bitcoin#23970: Remove pointless and confusing shift in RelayAddress (Kittywhiskers Van Gogh)
87205f26b5 merge bitcoin#21327: ignore transactions while in IBD (Kittywhiskers Van Gogh)
51ad8e4dde merge bitcoin#21148: Split orphan handling from net_processing into txorphanage (Kittywhiskers Van Gogh)
cbff29a630 partial bitcoin#20524: Move MIN_VERSION_SUPPORTED to p2p.py (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6098
* Dependent on https://github.com/dashpay/dash/pull/6233
* `p2p_ibd_txrelay.py` was first introduced in [bitcoin#19423](https://github.com/bitcoin/bitcoin/pull/19423) to test feefilter logic but on account of Dash not having feefilter capabilities, that backport was skipped over but on account of the tests introduced in [bitcoin#21327](https://github.com/bitcoin/bitcoin/pull/21327) that test capabilities present in Dash, a minimal version of `p2p_ibd_txrelay.py` has been committed in.
* `vSendMsg` is originally a `std::deque` and as an optimization, was changed to a `std::list` in 027a852a ([dash#3398](https://github.com/dashpay/dash/pull/3398)) but this renders us unable to backport [bitcoin#26844](https://github.com/bitcoin/bitcoin/pull/26844) as it introduces build failures. The optimization has been reverted to make way for the backport.
<details>
<summary>Compile failure:</summary>
```
net.cpp:959:20: error: invalid operands to binary expression ('iterator' (aka '_List_iterator<std::vector<unsigned char, std::allocator<unsigned char>>>') and 'int')
if (it + 1 != node.vSendMsg.end()) {
~~ ^ ~
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_bvector.h:303:3: note: candidate function not viable: no known conversion from 'iterator' (aka '_List_iterator<std::vector<unsigned char, std::allocator<unsigned char>>>') to 'ptrdiff_t' (aka 'long') for 1st argument
operator+(ptrdiff_t __n, const _Bit_iterator& __x)
[...]
1 error generated.
make[2]: *** [Makefile:11296: libbitcoin_server_a-net.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:19171: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:799: all-recursive] Error 1
```
</details>
* The collection of `CNode` pointers in `CConnman::SocketHandlerConnected` has been changed to a `std::set` to allow for us to erase elements from `vReceivableNodes` if the node is _also_ in the set of sendable nodes and the send hasn't entirely succeeded to avoid a deadlock (i.e. backport [bitcoin#27981](https://github.com/bitcoin/bitcoin/pull/27981))
* When backporting [bitcoin#28165](https://github.com/bitcoin/bitcoin/pull/28165), `denialofservice_tests` has been modified to still check with `vSendMsg` instead of `Transport::GetBytesToSend()` as changes in networking code to support LT-SEMs (level-triggered socket events mode) mean that the message doesn't get shifted from `vSendMsg` to `m_message_to_send`, as the test expects.
* Specifically, the changes made for LT-SEM support result in the function responsible for making that shift (`Transport::SetMessageToSend()` through `CConnman::SocketSendData()`), not being called during the test runtime.
* As checking `vSendMsg` (directly or through `nSendMsgSize`) isn't enough to determine if the queue is empty, we now also check with `to_send` from `Transport::GetBytesToSend()` to help us make that determination. This mirrors the change present in the upstream backport ([source](https://github.com/bitcoin/bitcoin/pull/28165/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1324-R1327)).
## Breaking Changes
* `bandwidth.message.*.bytesSent` will no longer include overhead and will now only report message size as specifics that let us calculate the overhead have been abstracted away.
## 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 76a458e5f9
Tree-SHA512: 2e47c207c1f854cfbd5b28c07dd78e12765ddb919abcd7710325df5d253cd0ba4bc30aa21545d88519e8acfe65638a57db4ca66853aca82fc355542210f4b394
To allow for the removal of a node from `vReceivableNodes`, the
collection of node pointers have been made into an `std::set`.
Marking as partial as it should be revisited when bitcoin#24356 is
backported.
The change was introduced as an optimization in 027a852a (dash#3398) but
prevents the backport of bitcoin#26844 due to the inability to engage in
binary expressions with iterators of `std::list`.
`p2p_ibd_txrelay.py` was introduced in bitcoin#19423 but not backported
as Dash doesn't have feefilter capabilities but this backport has the
test check for additional cases which are within Dash's capabilities, so
the test has been committed in with the feefilter portions minimally
stripped out
ef4d74a669 test: remove dead code from `p2p_initial_headers_sync.py` to favor of disable mocktime (UdjinM6)
4d9837c21e refactor: add a new flag disable_mocktime to set_test_params() (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
To disable mocktime you should re-implement setup_nodes(). It seems as bug-friendly solution
## What was done?
This PR introduce a new flag "disable_mocktime" which can be set in `set_test_params`.
It seems more error prune and the code is shorter
## How Has This Been Tested?
Run unit/functional tests including future changes from https://github.com/dashpay/dash/pull/6235
## 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
- [x] 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
ACKs for top commit:
UdjinM6:
utACK ef4d74a669
kwvg:
utACK ef4d74a669
PastaPastaPasta:
utACK ef4d74a669
Tree-SHA512: c6be8002cae4d7824e150938957464c156931d0b6f7fc41c430a83d662865431f1b56cb11df73f56db54a140f36b0addd68b2917e25c5c941fae52e8d322bc25
`NodeContext` member was moved from `TestingSetup` to
`BasicTestingSetup` in bitcoin#18571 but `connman` (which was a remnant
from when `NodeContext` was absent) wasn't removed.
Let's resolve that.
We need to continue inheriting the existing set of arguments to prevent
block invalidation due to missing arguments that allow for fast DIP3
activation (will manifest as `bad-qc-premature`)
be5c84f41d Merge bitcoin/bitcoin#22089: test: MiniWallet: fix fee calculation for P2PK and check tx vsize (MarcoFalke)
247141d32c Merge bitcoin/bitcoin#22210: test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
dad3ae3f33 Merge bitcoin/bitcoin#22130: test: refactor: dedup utility function chain_transaction() (MarcoFalke)
9dff334f47 Merge bitcoin-core/gui#361: Fix gui segfault caused by bitcoin/bitcoin#22216 (Hennadii Stepanov)
1087849955 Merge bitcoin/bitcoin#22216: refactor: Make SetupServerArgs callable without NodeContext (MarcoFalke)
fd94de6888 Merge bitcoin/bitcoin#21178: test: run mempool_reorg.py even with wallet disabled (MarcoFalke)
f1f5723fcf fix: missing changes from bitcoin#14123 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v22
## What was done?
See commits for list of backported changes. It also have some missing changes from bitcoin#14123
## 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:
UdjinM6:
utACK be5c84f41d
PastaPastaPasta:
utACK be5c84f41d
Tree-SHA512: 553ffde63c8409799cf6b3b87bf1ee285fbf58b13c08d04cdac29bc0e4dd75059feaa2f163803084ae85175397512517b68a6e0e0cc602d981f38ac70d96e393
f5b29e3227 chore: bump version on develop to 21.2 (pasta)
Pull request description:
## Issue being fixed or feature implemented
Currently, builds such as nightlies still think they are 21.1, but they're not. Bump it to 21.2. We should be continually updating this in develop to reflect what we expect next version to be
## What was done?
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] 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:
kwvg:
utACK f5b29e3227
UdjinM6:
utACK f5b29e3227
Tree-SHA512: 4bd6d51a247f7c889284da94a4c80c4de43787d8dae62c5b24ad41addadb599e4921149e74b93533d4353c31e9c8ef01cbfc8ec3060f0aded4132703a1cf8412
b75e83b298 merge bitcoin#24218: Fix implicit-integer-sign-change (Kittywhiskers Van Gogh)
8ecc22f51f merge bitcoin#23471: Improve ZMQ documentation (Kittywhiskers Van Gogh)
2965093c4a merge bitcoin#22079: Add support to listen on IPv6 addresses (Kittywhiskers Van Gogh)
3ac3714957 merge bitcoin#21310: fix sync-up by matching notification to generated block (Kittywhiskers Van Gogh)
7b0c725c59 merge bitcoin#21008: fix zmq test flakiness, improve speed (Kittywhiskers Van Gogh)
5e87efd04b merge bitcoin#20523: deduplicate 'sequence' publisher message creation/sending (Kittywhiskers Van Gogh)
99c730f0f3 merge bitcoin#20953: dedup zmq test setup code (node restart, topics subscription) (Kittywhiskers Van Gogh)
982c1f03d4 merge bitcoin#19572: Create "sequence" notifier, enabling client-side mempool tracking (Kittywhiskers Van Gogh)
b0b4e0fa7f zmq: Make `g_zmq_notification_interface` a smart pointer (Kittywhiskers Van Gogh)
0a1ffd30b9 zmq: extend appending address to log msg for Dash-specific notifications (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572) introduces tests in `interface_zmq.py` that validate newly introduced "sequence" reporting of all message types (`C`, `D`, `R` and `A`). The `R` message type (removed from mempool) is tested by leveraging the RBF mechanism, which isn't present in Dash.
In order to allow the tests to successfully pass, all fee bumping and RBF-specific code had to be removed from the tests. This test also involves creating a new block to test the `C` message (connected block) that would return a `time-too-new` error and fail unless mocktime has been disabled (which has been done in this test).
* When backporting [bitcoin#18309](https://github.com/bitcoin/bitcoin/pull/18309) ([dash#5728](https://github.com/dashpay/dash/pull/5728)), Dash-specific ZMQ notifications did not have those changes applied to them and that particular backport was merged upstream *after* [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572), meaning, for completion, the changes from [bitcoin#18309](https://github.com/bitcoin/bitcoin/pull/18309) have been integrated into [bitcoin#19572](https://github.com/bitcoin/bitcoin/pull/19572)'s backport.
As for the Dash-specific notifications, they have been covered to ensure uniformity in a separate commit.
* The ZMQ notification interface has been converted to a smart pointer in the interest of safety (this change is eventually done upstream in 8ed4ff8e05d61a8e954d72cebdc2e1d1ab24fb84 ([bitcoin#27125](https://github.com/bitcoin/bitcoin/pull/27125)))
## Breaking Changes
None expected.
## 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
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK b75e83b298
PastaPastaPasta:
utACK b75e83b298
knst:
utACK b75e83b298
Tree-SHA512: 9f860d1203bebe0914a5102f101f646873d14754830d651fb91ed0d1285a6c1a58ffc492b07d4768324d94f53171c9a4da974cf4a0b1e5c665979eace289f6f0
5aefd44ea3 fmt: run clang-format (pasta)
7e9dec290f refactor: drop some unneeded `this` and `const` (pasta)
Pull request description:
## Issue being fixed or feature implemented
Neither identifies are used for anything, the const is always gonna be discarded as it's on a value return value, and the `this` is not used, so should be dropped.
This fixes some spammy warnings for me locally
## What was done?
## How Has This Been Tested?
Compiled in CI
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] 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 5aefd44ea3
UdjinM6:
utACK 5aefd44ea3
Tree-SHA512: 9ebfae9bccc8893dc79c25d2b824680f63d27ab2da82089c96660eacb5891577e00145f6b70a20b161fc6fda1b6e5cfd6897502ffd356f8febcec76844623613
c2c4b2b794 fix: release unused memory in `CNetMsgMaker::Make()` (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
We reserve capacity for large messages in `CNetMsgMaker::Make()` but most messages are small yet we never release unused memory here. Discovered while debugging 28165 backport issues.
## What was done?
## How Has This Been Tested?
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone
ACKs for top commit:
kwvg:
ACK c2c4b2b794
PastaPastaPasta:
utACK c2c4b2b794
knst:
utACK c2c4b2b794
Tree-SHA512: 72a3728e316fb76ca135fbfff4f15b80c60b048d7e916d2f5dbcde4b64dce7177af80f8f24153636ccfab2f5d5c7d9edc5b3bdba121f64c8da03117a98fd7411
2eadcf2f68 partial Merge bitcoin/bitcoin#29007: test: create deterministic addrman in the functional tests (stratospher)
f95ca4ed3e fix: unify with bitcoin: removed requirement of txindex=0 (Konstantin Akimov)
bf46e7a8e1 partial Merge bitcoin/bitcoin#28799: wallet: cache descriptor ID to avoid repeated descriptor string creation (Andrew Chow)
05e5966199 partial Merge bitcoin/bitcoin#27920: wallet: bugfix, always use apostrophe for spkm descriptor ID (furszy)
Pull request description:
## Issue being fixed or feature implemented
Lately our CI for tsan is flapping many functional tests and take long times.
This PR has several important changes backported from the latest bitcoin's version to improve CI experience
## What was done?
This PR has several backports that improved CI experience drastically.
**Firstly, it aims to fix flapping test p2p_node_network_limited.py**
For example: https://gitlab.com/dashpay/dash/-/jobs/7692635307
<details>
<summary>p2p_node_network_limited.py | ✖ Failed | 28 s</summary>
```
test 2024-08-29T02:50:53.929000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_framework.py", line 158, in main
self.run_test()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/p2p_node_network_limited.py", line 79, in run_test
self.nodes[0].disconnect_p2ps()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_node.py", line 611, in disconnect_p2ps
wait_until_helper(check_peers, timeout=5)
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/util.py", line 262, in wait_until_helper
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
def check_peers():
for p in self.getpeerinfo():
for p2p in self.p2ps:
if p['subver'] == p2p.strSubVer:
return False
return True
''' not true after 5.0 seconds
```
</details>
**Secondly, it improves performance of Descriptor wallets significantly for case of `tsan` CI**. It is tiny improvement for Release build and local runs, but some fucnctional tests run as fast as twice:
https://gitlab.com/dashpay/dash/-/jobs/7694458953https://gitlab.com/dashpay/dash/-/jobs/7665132625
```
wallet_create_tx.py --descriptors | ✓ Passed | 236 s <-- new version
wallet_create_tx.py --legacy-wallet | ✓ Passed | 108 s
wallet_basic.py --descriptors | ✓ Passed | 135 s <---- new version
wallet_basic.py --legacy-wallet | ✓ Passed | 97 s
wallet_create_tx.py --descriptors | ✓ Passed | 456 s <-- old version
wallet_create_tx.py --legacy-wallet | ✓ Passed | 98 s
wallet_basic.py --descriptors | ✓ Passed | 189 s <--- old version
wallet_basic.py --legacy-wallet | ✓ Passed | 131 s
```
See performance investigation here: https://github.com/dashpay/dash/pull/6226
## 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 2eadcf2f68
UdjinM6:
utACK 2eadcf2f68
Tree-SHA512: 127fbaa65c160aa95e2145a6b40d3811f7c42e36fbee9ce98a9ac021abd9cbe6edc7791870b331a54855ba891e3804885db7936ef212647b693f50f79a60d232
BACKPORT NOTICE
It includes only this commit: be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac
[init] Remove -addrmantest command line arg
-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.
Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
BACKPORT NOTICE:
It doesn't include changes related to wallet_miniscript.py
5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7 test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner (Sebastian Falbesoner)
f811a24421fe102a96ab8f75427cc6a3c5503dc3 wallet: cache descriptor ID to avoid repeated descriptor string creation (Sebastian Falbesoner)
Pull request description:
Right now a wallet descriptor is converted to its string representation (via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like `FastWalletRescanFilter` etc.
As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that.
This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.
Fixes https://github.com/bitcoin/bitcoin/issues/28800.
ACKs for top commit:
Sjors:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
S3RK:
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
achow101:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
BrandonOdiwuor:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
Tree-SHA512: 98b43963a5dde6055bb26cecd3b878dadd837d6226af4c84142383310495da80b3c4bd552e73b9107f2f2ff1c11f5e18060c6fd3d9e44bbd5224114c4d245c1c
BACKPORT NOTICE
It includes only this commit: 97a965d98f1582ea1b1377bd258c9088380e1b8b
refactor: extract descriptor ID calculation from spkm GetID()
This allows us to verify the descriptor ID on the descriptors
unit tests in different software versions without requiring to
use the entire DescriptorScriptPubKeyMan machinery.
Note:
The unit test changes are introduced after the bugfix commit
but this commit + the unit test commit can be cherry-picked
on top of the v25 branch to verify IDs correctness. IDs must
be the same for v25 and after the bugfix commit.
