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.
0f7dc893ea1776515173dcd0bfe6826e963c90f3 test: compare `/chaininfo` response with `getblockchaininfo` RPC (brunoerg)
Pull request description:
The `/chaininfo` REST endpoint gets its infos from `getblockchaininfo` RPC, so this PR adds an `assert_equal` (in `interface_rest`) to ensure both responses are the same. Obs: other endpoints do the same for their respective RPC.
ACKs for top commit:
0xB10C:
Concept and Code Review ACK 0f7dc893ea1776515173dcd0bfe6826e963c90f3. Belts-and-spenders.
Tree-SHA512: 51cbcf988090272e406a47dc869710740b74e2222af29c05ddcbf53bd49765cdc59efb525e970867f091b3d2efec4fb13371a342d9e484e51144b760265bc5b8
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
47c48b5f35b4910fcf87caa6e37407e67d879c80 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b189e80f2c140db4df50177353a1cb83 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8a8f9c693c0ea67114bbff24a0754df test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
accf3d5868460b4b14ab607fd66ac985b086fbb3 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c24d2f0abd442c1c33098e3121572ce [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7beddfdb74c284720d209c81dfdb94f [test] helper function to increase transaction weight (glozow)
f8253d69d6f02850995a11eeb71fedc22e6f6575 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d33aa7ef87994e452bced7f192d021a0 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa139c8363e9b38bbc1ba0dca55700b8a7 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3ec080a2d7a7988c7b35088dff6c5830 [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d2bbedf9527b914c0cc8267b8a7c21b MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aaf72aab8a9c5827b1f162f3b8ac38f4 misc package validation doc improvements (glozow)
Pull request description:
This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).
Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.
#### Motivation
It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:
- We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
- We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.
ACKs for top commit:
JeremyRubin:
utACK accf3d5
ariard:
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
dc59cbc315 fix: optimize includes in gsl/pointer.h to speed up compile time (Konstantin Akimov)
d5e32e1b79 fix: disable gsl iostream feature for faster compile time (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
This header `gsl/pointers.h` is included in multiple other headers all over codebase.
Any extra line of code inside gsl/pointers.h makes all project to compile slower.
## What was done?
Removed headers `<algorithm>`, `<system_error>`, `<iosfwd>` from gsl/pointers.h
## How Has This Been Tested?
Run command 5 times, takes minimum time:
- baseline
```
$ time g++ -o /tmp/a.out gsl/pointers.h -O2 -g -I.
real 0m0,572s
user 0m0,461s
sys 0m0,108s
```
- removed algorithm:
```
$ time g++ -o /tmp/a.out gsl/pointers.h -O2 -g -I.
real 0m0,505s
user 0m0,398s
sys 0m0,107s
```
- removed algorithm and system_error:
```
real 0m0,332s
user 0m0,265s
sys 0m0,067s
```
- disabled iostream:
```
$ time g++ -o /tmp/a.out gsl/pointers.h -O2 -g -I. -D GSL_NO_IOSTREAMS
real 0m0,316s
user 0m0,256s
sys 0m0,060s
```
as a result, overall project compilation time is also improved: `make clean ; sleep 3s ; time make -j20`
```
real 5m42,934s
user 80m35,127s
sys 6m40,735s
```
```
real 5m28,862s
user 75m31,931s
sys 6m32,591s
```
## 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
ACKs for top commit:
UdjinM6:
utACK dc59cbc315
Tree-SHA512: 353d79c0e297c92e823972b53daaaed42494554ce550ea800ce8aa6990c704cedfbcb922e1d735eb8fc01ad14ffa873d86cdbc4d3731d841496392bb74286d33
1f4e1a17ed test: add test for governance inv expiration (UdjinM6)
06b4dba0d3 feat: make CInv python implementation aware of governance invs (UdjinM6)
c7c930ece6 fix: use correct condition in logs (UdjinM6)
d41d87a5be fix: use `std::chrono::seconds` (UdjinM6)
d6fe7146ff chore: make clang-format and linter happy (UdjinM6)
b57a9220c1 refactor: sqash 2 hash maps into one, use proper naming (UdjinM6)
4116ba3253 fix: let internal govobj/vote inv request trackers expire after 60 sec (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Another issue noticed during recent sb voting period. Lots of inv are coming from peers that never send the object/vote they announced. Let's not wait for these forever, 60 seconds should be enough.
## What was done?
Add a "timer" to each request.
## How Has This Been Tested?
Run tests, run a MN on mainnet and check logs.
## 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 _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 1f4e1a17ed
Tree-SHA512: f5c5a896f61b5aed27c6f42c926254156c604edb4efd2a3cd3738a7e8d1ed7bafffadc584148e4a5c1a172c2b3d61a06884a1f6d56eb4ebe37a4b0dbc050edd6
87d775d27c partial bitcoin#22868: Call load handlers without cs_wallet locked (Kittywhiskers Van Gogh)
c602ca15e1 coinjoin: protect `m_wallet_manager_map` with `cs_wallet_manager_map` (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
This pull request aims to deal with regressions ([build](https://gitlab.com/dashpay/dash/-/jobs/7550859495)) spotted in `develop` after the merger of [dash#6143](https://github.com/dashpay/dash/pull/6143), namely, a failure to build the multiprocess variant and two data races, one involving `ChainstateManager::m_best_header` and another involving `CoinJoinWalletManager::m_wallet_manager_map`.
* Fixes for the build failure and the first data race (b136742f98c6792735232cc170123f849fd13bad and 9e7c6850139e69cb64cfe89813e6f0e4b1e626e1), have been spun off into [dash#6199](https://github.com/dashpay/dash/pull/6199) and merged
* The second data race is being avoided by protected `m_wallet_manager_map` with a new `RecursiveMutex`, `cs_wallet_manager_map` (the contents of this PR). ~~A version of these changes are available using a regular `Mutex` but prove far more cumbersome ([source](b89457dc9a)) when taking practicalities into account ([comment](https://github.com/dashpay/dash/pull/6192#discussion_r1713207664)).~~ A `Mutex` version is now available courtesy of UdjinM6 (see [patch](ecf5c1ba76)), thanks!
## 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:
UdjinM6:
light-ACK 87d775d27c
Tree-SHA512: 52545a1547357a45fb6bf4d3948fc75a5e88f1d86e9809b1060007cd74dd383249691d8d9777f647c7534d458fe126ae818fa2b47f0e5b0cee78a469af1ed0c9
1973532372 Merge bitcoin/bitcoin#22337: wallet: Use bilingual_str for errors (Samuel Dobson)
Pull request description:
bitcoin backport
Top commit has no ACKs.
Tree-SHA512: ec638bde047788824961c230804461cee96ddcc0dbe1928bfb8c675c7f7f8c0c104850676a70fb8c9380401ead6d5986022ae3698a2bbc84f679531b11349580
d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0ef2dcd307023aaaab40373c8bf17db1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
fa7d71f270b89c9d06230d4ff262646f9ea29f4a test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a4b02a41b7448b45f0e6cdfdbb53ac3 test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649e3da4a5a5f6cffac3fc748bb1ca900 test: Remove unused generate() from test (MarcoFalke)
faff3f35b778d9af3d649b303d7edab49bfe40b4 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups 🎨
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a thanks for the explanations, nicely done
theStack:
ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a 🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
01eedf3821f2c3ee6bab8733f8549531c844add7 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner)
6e63e366d609312ab984b4439de2d59bb618620b test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner)
Pull request description:
Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:
* pass a list of parent_txids/vouts instead of single values
* always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one)
This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.
ACKs for top commit:
mjdietzx:
reACK 01eedf3821f2c3ee6bab8733f8549531c844add7
klementtan:
Code review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7
MarcoFalke:
review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7 🙅
Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0
493fb47c577b7564138c883a8f22cbac3619ce44 Make SetupServerArgs callable without NodeContext (Russell Yanofsky)
Pull request description:
`bitcoin-gui` code needs to call `SetupServerArgs` but will not have a `NodeContext` object if it is communicating with an external `bitcoin-node` process, so this just passes `ArgsManager` directly.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
MarcoFalke:
review ACK 493fb47c577b7564138c883a8f22cbac3619ce44
Tree-SHA512: 94cda4350113237976e32f1935e3602d1e6ea90c29c4434db2094be70dddf4b63702c3094385258bdf1c3e5b52c7d23bbc1f0282bdd4965557eedd5aef9a0fd4
a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47 test: run mempool_reorg.py even with wallet disabled (Darius Parvin)
Pull request description:
Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.
As part of this PR I created a new method in `MiniWallet`, `create_self_transfer`, to return a raw tx (without broadcasting it) and its associated utxo.
ACKs for top commit:
MarcoFalke:
cr ACK a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47
Tree-SHA512: 316a38faffadcb87499c1d6eca21e9696cef65362bbffcf621788a9b771bb1fa2971b1c7835cbd34b952d7612ad83afbca824cd8be39ecd6b994e8963027f991
92993aa5cf37995e65e68dfd6f129ecaf418e01c Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b828a557f8262d9dc14ff7a03f813f7 Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51115454c6a699be9492024f7b46c2b4 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
meshcollider:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
Signed-off-by: Vijay <vijaydas.mp@gmail.com>
