5a157eb3703e1e65ed285f13a824562ab12aa3ff Bugfix: configure: Only avoid -isystem for exact /usr/include path (Luke Dashjr)
556ee6f2fa0e2da2bb12fe05a885431f61db2e47 Bugfix: configure: Quote SUPPRESS_WARNINGS sufficiently to preserve brackets (Luke Dashjr)
Pull request description:
The regex includes `[/ ]` which is supposed to match either a forward slash or a space, but m4 treats the brackets as special characters and effectively strips them out, leading to -isystem /usr/include paths except for in the typical scenario where it is the final parameter in the flag string.
ACKs for top commit:
hebasto:
ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff, tested on Ubuntu 22.04 with clang 14.0.
vasild:
ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff
Tree-SHA512: 5c8c282b647b7853b8fad1b5b473703c4a0635073d2685a8ac984151046e2c6a859e6972465419d27356dd29a47f21a2a3a6ad402ec434fe1f9882e5a35f0749
f8cba0d9117fe9b9ac51d7044372b28270c7838b test: Change default test logging directory (Yancy Ribbens)
Pull request description:
This PR changes the default test log location request here: https://github.com/bitcoin/bitcoin/issues/17224. Instead of using the location of the makefile [automatic variable](https://www.gnu.org/software/make/manual/make.html#Automatic-Variables) `$<` I extract just the basename and then prepend a new location `./test`. This is done because `$<` represents the variable name AND location of the prerequisite here.
Top commit has no ACKs.
Tree-SHA512: f0fbc530cf0e14c284b4bbf6671c145b1d7a2e1f5561f5c5d09f0cbe88b98e620e763bbbf2dfa9aeeec3dcc9b0127939e105e14c7e4f6660c7c19663622a393d
28f17c1a6de7b50482d8f55fe03698126862e8eb build: fix copypasta in OpenBSD C{XX} flags (fanquake)
Pull request description:
Introduced in #23998.
ACKs for top commit:
hebasto:
ACK 28f17c1a6de7b50482d8f55fe03698126862e8eb, I have reviewed the code and it looks OK, not tested on OpenBSD though.
Tree-SHA512: d905161534075f518c8924d3c42cca7ff8d4898e559f1daa9bd03dac95b109b2c3e76790fb8bc65b9e45e8a59566825afbf4dc3734ad74617dfdf797430e486b
061accfddd2d27ad584c826413c68857d2be9ced build: require libtool 2.4.2 (fanquake)
Pull request description:
Every system we support has 2.4.6 available, except for OpenBSD, which
[currently ships with 2.4.2](https://github.com/openbsd/ports/blob/master/devel/libtool/Makefile) (released 2011). For now, set our minimum
required version to that.
After a 7 year hitus, 2.4.7 has also very recently been released:
https://savannah.gnu.org/forum/forum.php?forum_id=10139.
Partially motivated by comments in https://github.com/bitcoin/bitcoin/pull/24615.
See also: https://repology.org/project/libtool/versions
ACKs for top commit:
achow101:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
hebasto:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
prusnak:
ACK 061accfddd2d27ad584c826413c68857d2be9ced
Tree-SHA512: bc032022b8609b73253ff1c4fd480f4d09be761b8fec295f39319f9499ee2df116f55295da476be551c43ed88fbb0bfed7bb5a188b9979b34147fe39737ec76f
38a1b0b1967272cbb984f37bcc86b6640930319f doc: remove unneeded documentation on basic package management on FreeBSD (jessebarton)
Pull request description:
In reference to #24618
ACKs for top commit:
fanquake:
ACK 38a1b0b1967272cbb984f37bcc86b6640930319f - Thanks. In future, please re-use existing PRs, so that discussion and changes are kept together.
Tree-SHA512: ece5b85bca7f11e11d47c0674a6b96a72c3bb65dd02ab25553db511a001a9fc682c0ff8276e39d979fdd1f57a64137f586cfa548aab5c08cd9341455217b9181
339b4a51f6d3558c3489b14efe0c8c195295cf86 build: don't install deprecated libevent headers (fanquake)
Pull request description:
We don't use the deprecated headers now, and never should do in the
future, so there is no need for them to exist in depends.
The headers themselves are just full of includes for the newer headers.
ACKs for top commit:
hebasto:
ACK 339b4a51f6d3558c3489b14efe0c8c195295cf86
Tree-SHA512: 736fd9e3b22212da462cc05203dd253806dc59f973090357b705f2742ed4a3b8c3cc44b3173d706527f60ad93e95cf4143ec6b7db4233a489890a98f8e5c8f07
e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov)
Pull request description:
bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly.
This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again.
Fixes bitcoin-core/gui#545.
ACKs for top commit:
RandyMcMillan:
tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3
Sjors:
tACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 (rebased on master) indeed fixes the crash described in #545
promag:
Tested ACK e7fc50681e99e3c726db2bc4d3d425ed8a0fc6b3 on macOS 10.15 with Qt 5.15.2.
Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131
faa7d8a3f7cba02eca7e247108a6b98ea9daf373 util: Add SaturatingAdd helper (MarcoFalke)
Pull request description:
Seems good to have this in the repo, as it might be needed to write cleaner code. For example:
* https://github.com/bitcoin/bitcoin/pull/24090#issuecomment-1019948200
* https://github.com/bitcoin/bitcoin/pull/23418#discussion_r744953272
* ...
ACKs for top commit:
MarcoFalke:
Added a test. Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa90189cbf faa7d8a3f7`
klementtan:
reACK faa7d8a3f7
vasild:
ACK faa7d8a3f7cba02eca7e247108a6b98ea9daf373
Tree-SHA512: d0e6efdba7dfcbdd16ab4539a7f5e45a97d17792e42586c3c52caaae3fc70612dc9e364359658de5de5718fb8c2a765a59ceb2230098355394fa067a9732bc2a
71d33380ed6858b4a65b396332bfb22d984642a6 qt: prevent negative values of progressPerHour (HiLivin)
Pull request description:
Added a similar guard to _progressPerHour_ as is placed at _remainingMSecs_.
It prevents the display of negative values like "-0.00%" in some cases.
ACKs for top commit:
hebasto:
ACK 71d33380ed6858b4a65b396332bfb22d984642a6
jarolrod:
ACK 71d3338
shaavan:
reACK 71d33380ed6858b4a65b396332bfb22d984642a6
Tree-SHA512: 5427cdf4441b542196008034355ea00a075adf8b9aeeb383bacdb4e5fbda23d665448a50035aac93cbf401d5d6211d39a2c7c294568d9f5548a5c7579e201c44
faa5e171e6bdb8f3b4027a3f06497f0de5abf766 RPCConsole: Throw when overflowing size_t type for array indices (MarcoFalke)
Pull request description:
To test:
-> `getblock(getbestblockhash(), 1)[tx][22222222222222222222222222222]`
Before:
<- `868693731dea69a197c13c2cfaa41c9f78fcdeb8ab8e9f8cdf2c9025147ee7d1` (hash of the coinbase tx)
After:
<- `Error: Invalid result query`
ACKs for top commit:
jarolrod:
ACK faa5e171e6bdb8f3b4027a3f06497f0de5abf766
shaavan:
ACK faa5e17
Tree-SHA512: ddff39aae1c15db45928b110a9f1c42eadc5404cdfa89d67ccffc4c6af24091967d43c068ce9e0c1b863cfc4eb5b4f12373a73756a9474f8294e8a44aabc28d8
9bd168bf5457c6fd9770769547d8757bf14813b0 qt: add missing tooltips to options menu settings (Jarol Rodriguez)
Pull request description:
This adds missing tooltips to the text of the `Size of database cache` and the `Number of script verification threads` settings.
All settings in the Options window will now have appropriate tooltip texts.
ACKs for top commit:
jonatack:
ACK 9bd168bf5457c6fd9770769547d8757bf14813b0 tested on Debian 5.10.46-4 (2021-08-03)
hebasto:
ACK 9bd168bf5457c6fd9770769547d8757bf14813b0, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: d71946bfee33c624a8b79eafe514d2c902090a40bc25097be4c7da4a80270f53305002af1b27d5fd082a0f45f838e22036632f9445918c4b8898073b33c09c08
93cc53a2b27eeb05fe00b8bf17465037815473a1 gui: Unregister wallet notifications before unloading wallets (Russell Yanofsky)
Pull request description:
This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.
ACKs for top commit:
promag:
Code review ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1.
hebasto:
ACK 93cc53a2b27eeb05fe00b8bf17465037815473a1
Tree-SHA512: 805f50a493291ad0f7c48725fbc5058d58ebbdb0770befd51d8aa241209a13f8a46f5982481336ab8338cdc83e9017668089a71deccf1587308e841cf8697825
87651795d8622d354f8e3c481eb868d9433b841c fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517f3418e2a408071b4c2ce26571f3167 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)
Pull request description:
Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
ACKs for top commit:
practicalswift:
cr ACK 87651795d8622d354f8e3c481eb868d9433b841c
jonatack:
ACK 87651795d8622d354f8e3c481eb868d9433b841c rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
bcd36e14f0a2b89445f1436094de40696d6ad76c build: correct depends FreeBSD C{XX}FLAGS (fanquake)
7b06ffce9c50110b475c722918c55a14402346a5 build: add NetBSD support to depends (fanquake)
Pull request description:
Similar to #23948. Doesn't build the Qt package; I haven't looked at doing that yet, but have an assumption that it's going to fail out of the box similar to the FreeBSD build.
Guix Build:
```bash
```
ACKs for top commit:
theuni:
ACK bcd36e14f0a2b89445f1436094de40696d6ad76c
Tree-SHA512: 9a0946cefbcb9a92dd730b885463f3213e304c8d4b39fea8d831fc013a73d2ef998ca84e384bf45a01fa1449cf5a35eaffaa5b57a9062c2cdda34312d33ec3fc
fadd73037e266edb844f0972e82e9213171ef214 refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e266edb844f0972e82e9213171ef214
theStack:
Code-review ACK fadd73037e266edb844f0972e82e9213171ef214
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
b062da009001c1beb362169d700663d7220eef5e contrib: add check for wget command in install_db4.sh (Florian Baumgartl)
Pull request description:
This PR is motivated by 7bb8eb0bc3 commit (see also https://github.com/bitcoin/bitcoin/pull/23579) and ensures that `install_db4.sh` will check for `curl` and `wget` utilities. Currently, the conditional statement in the `http_get()` function assumes that `wget` is always available but we actually do not know it since there is no check or validation for the `wget` command. So let's make sure that we check for both commands and print an error message if they are missing.
ACKs for top commit:
jamesob:
ACK b062da0090
laanwj:
Tested ACK b062da009001c1beb362169d700663d7220eef5e
shaavan:
ACK b062da009001c1beb362169d700663d7220eef5e
Tree-SHA512: bfc1ccad9a5b99764b759e02dde1976616c2af4747b7d5af8e71d33624c2cb21d93a09a60d244756e86bbd5fd7541331c62d7eb84d3458b6a059f1d9cb2a5f42
c055f6b216659b844c8dcd4ff2a977f181099678 test: Remove false coinstatsindex test (Fabian Jahr)
Pull request description:
This test never actually tested the behavior that it describes in the comments. This was discovered in #21590 which seems to speed up muhash which lead to the test failing.
I can vaguely remember that the described behavior was desired by some reviewers of `coinstatsindex`: That `coinstatsindex` should be aware of stale blocks and able to return statistics on them as well. The index actually does this for blocks that it sees while the index is active, i.e. while running `coinstatsindex` all blocks will be indexed and even when they become stale the index (via `gettxoutsetinfo`) will still return a result for them when given the right hash. But this currently does not work for blocks that the node saw and that became stale _before_ the node activated `coinstatsindex`. While the index syncs initially everything but the active chain is ignored and I don't see any indication that this ever worked differently in the past.
Introducing this behavior seems non-trivial at first glance so, while I will give this a shot, I think the test should be removed so it does not confuse users and does not block #21590.
Top commit has no ACKs.
Tree-SHA512: b05f5dfeea3e453c8bb7c761501d0d896d4412a3f0c08037955951fae9fe388c63402da401792591e18da8fb67734f47f1a297d573cdb66e0ced451698718067
7bb8eb0bc352b47ee962283898f9becbb4f36c62 script install_db4.sh added check for patch command (Nathan Garabedian)
Pull request description:
First contribution. I've read the CONTRIBUTING guide and hope I'm doing this correctly, but please kindly point out anything I should do differently.
I found while running the contrib/install_db4.sh patch that it would fail suddenly with "patch: command not found". I'd rather see it fail early before doing any work, allow me to install the `patch` command, and then run again. (CentOS Linux) Here's a PR proposed to fix it.
error message:
```
...
db-4.8.30.NC/txn/txn_rec.c
db-4.8.30.NC/txn/txn_region.c
db-4.8.30.NC/txn/txn_stat.c
db-4.8.30.NC/txn/txn_util.c
./contrib/install_db4.sh: line 71: patch: command not found
```
ACKs for top commit:
laanwj:
Code review and tested ACK 7bb8eb0bc352b47ee962283898f9becbb4f36c62
Tree-SHA512: f0c59ec509dff6637c41eec2923fe708456c3a7ae04495b12c5492681a1f95d1d9a643a2649df13ba8e6ac708680c627657b6989b62eff63be021e92e1d7c4a8
33c6a208a9e2512a174c99c224a933a59f091bc2 span, doc: provide span.h context and explain lifetimebound definition (Jon Atack)
d14395bc5db55331115fa3c1e71741d1de7f092f net, doc: provide context for UnserializeV1Array() (Jon Atack)
Pull request description:
Add contextual documentation for developers and future readers of the code regarding
- CNetAddr::UnserializeV1Array (see #22140)
- Span and why it defines Clang lifetimebound locally rather than using the one in attributes.h
ACKs for top commit:
laanwj:
Documentation review ACK 33c6a208a9e2512a174c99c224a933a59f091bc2
Tree-SHA512: cb8e6a6c23b36c9ef7499257e97c5378ec895bb9122b79b63b572d9721a1ae6ce6c0be7ad61bdf976c255527ae750fc9ff4b3e03c07c6c797d14dbc82ea9fb3a
ef72e9bd4124645fe2d00521a71c1c298d760225 doc: nChainTx needs to become a 64-bit earlier due to SegWit (Sjors Provoost)
Pull request description:
As of block 597,379 txcount is 460,596,047 (see `chainparams.cpp`), while `uint32` can handle up to 4,294,967,296.
Pre segwit the [minimum transaction size](https://en.bitcoin.it/wiki/Maximum_transaction_rate) was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for `unsigned int nChainTx` says, that should last until the year 2030.
With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see #15482), without a witness:
```
4 bytes version
1 byte input count
36 bytes outpoint
1 byte scriptSigLen (0x00)
0 bytes scriptSig
4 bytes sequence
1 byte output count
8 bytes value
1 byte scriptPubKeyLen
1 byte scriptPubKey (OP_TRUE)
4 bytes locktime
```
That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024.
Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it.
ACKs for top commit:
practicalswift:
re-ACK ef72e9bd4124645fe2d00521a71c1c298d760225
jarolrod:
ACK ef72e9bd4124645fe2d00521a71c1c298d760225
theStack:
ACK ef72e9bd4124645fe2d00521a71c1c298d760225
Tree-SHA512: d8509ba7641796cd82af156354ff3a12ff7ec0f7b11215edff6696e95f8ca0e3596f719f3492ac3acb4b0884ac4e5bddc76f107b656bc2ed95a8ef1b2b5d4f71
350e034e64d175f3db4c85ddca42e76e279912f6 consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack)
Pull request description:
Commit ccd8ef65 "Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock" in #11281 moved the cs_main lock from caller to `ReadBlockFromDisk()` for calling `CBlockIndex::GetBlockPos()`, but the second invocation doesn't have the lock, and IIUC there is no guarantee the compiler can know if state has changed.
Use the `blockPos` local variable instead, rename it to `block_pos`, and make it const.
ACKs for top commit:
laanwj:
Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6
theStack:
Code-review ACK 350e034e64d175f3db4c85ddca42e76e279912f6
promag:
Code review ACK 350e034e64d175f3db4c85ddca42e76e279912f6.
Tree-SHA512: 0df0614ab1876885c85f7b53c604a759a29008da8027e95503b4726d2b820ec6d27546020c613337ff954406e01cb5d191978ba4a12124052fed6e1b0e9a226f
317442525586ba9ff8b9af6c506b48f87cd8cd87 Cleanup headers after #20788 (Hennadii Stepanov)
Pull request description:
This is a header cleanup after #20788.
ACKs for top commit:
vasild:
ACK 317442525586ba9ff8b9af6c506b48f87cd8cd87
Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
56a42f10f452f0ac0e3e333646a8effcbebf6b30 Stricter BIP32 decoding and test vector 5 (Pieter Wuille)
Pull request description:
This adds detection for various edge cases when decoding BIP32 extended pubkeys/privkeys, and tests them using the proposed https://github.com/bitcoin/bips/pull/921 BIP32 test vector 5.
ACKs for top commit:
darosior:
utACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30 -- Had to implement essentially the same fix in python-bip32.
kristapsk:
ACK 56a42f10f452f0ac0e3e333646a8effcbebf6b30. Checked that test vectors are the same as in BIP32 and that tests pass.
Tree-SHA512: 5cc800cc9dc10e43ae89b659ce4f44026d04ec3cabac4eb5122d2e72ec2ed66cd5ace8c7502259e469a9ecaa5ecca2457e55dfe5fedba59948ecbf6673af67a7
## Issue being fixed or feature implemented
Cache population for old quorums is a cpu heavy operation and should be
avoided for inactive quorums _at least_ oin `ScanQuorums`. This issue is
critical for testnet and other small network because every mn
participate in almost every platform quorum and cache population for 2
months of quorums can easily block everything for 15+ minutes on a 4 cpu
node. On mainnet quorum distribution is much better but it's still a
small waste of cpu (or not so small for unlucky nodes).
#5761 follow-up
## What was done?
Do not start cache population for outdated quorums, improve logs in
`StartCachePopulatorThread` to make it easier to see what's going on.
## How Has This Been Tested?
run a mn on testnet
## 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)_
## Issue being fixed or feature implemented
`instantlock` and `chainlock` are broken in `getspecialtxes`
kudos to @thephez for finding the issue
## What was done?
pass the hash and also rename the variable to self-describing
## How Has This Been Tested?
run `getspecialtxes` on a node with and without the patch
## Breaking Changes
`instantlock` and `chainlock` will show actual values and not just
`false` all the time now (not sure if that qualifies for "breaking"
though)
## 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)_
## Issue being fixed or feature implemented
1. `scanQuorumsCache` is a special one and we use it incorrectly.
2. Platform doesn't really use anything that calls `ScanQuorums()`
directly, they specify the exact quorum hash in RPCs so it's
`GetQuorum()` that is used instead. The only place `ScanQuorums()` is
used for Platform related stuff is `StartCleanupOldQuorumDataThread()`
because we want to preserve quorum data used by `GetQuorum()`. But this
can be optimised with its own (much more compact) cache.
3. RPCs that use `ScanQuorums()` should in most cases be ok with smaller
cache, for other use cases there is a note in help text now.
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests, run a node (~in progress~ looks stable)
## 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
fabed982ad9143cddaca8346f6b4c243dd84e0c4 fuzz: Re-enable assert in banman again (MarcoFalke)
Pull request description:
Looks like this was accidentally fixed by removing the buggy code in commit efd6f904c78769ad2e93c1f1de43014d284e7561
ACKs for top commit:
hebasto:
ACK fabed982ad9143cddaca8346f6b4c243dd84e0c4
Tree-SHA512: 2dea5dad48ff2050ae7086c1c6306c40f650e9629918e79adc54164a375d777a70b29f5a480566dc6430f07ce33dfe703fc5d45a20125584b4a026c5832198a2
4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/22383#discussion_r675069128)):
b620b2d58a/src/txmempool.h (L554-L558)
`CTxMemPool::get()` acquires this lock:
b620b2d58a/src/txmempool.cpp (L822-L829)
so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked.
ACKs for top commit:
tryphe:
Concept ACK. tested 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f but not extensively.
jnewbery:
Code review ACK 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f
Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c
7b3a20b2602f902c344a615f23f8f0280b6f6bcc mempool: apply rule of 5 to epochguard.h, fix compiler warnings (Jon Atack)
Pull request description:
Apply the rule of five to `src/util/epochguard.h::{Epoch, Marker}` for safety, which also nicely fixes the `-Wdeprecated-copy` compiler warnings with Clang 13.
References:
- https://en.cppreference.com/w/cpp/language/rule_of_three
- https://www.stroustrup.com/C++11FAQ.html#default
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five
<details><summary>Compiler warnings fixed</summary><p>
```bash
In file included from policy/rbf.cpp:5:
In file included from ./policy/rbf.h:8:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
```
```bash
In file included from txmempool.cpp:6:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
node_alloc_traits::construct(
^
/usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here
final().construct_value(x,v);
^
/usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
final_node_type* res=super::insert_(v,x,variant);
^
/usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
return insert_(v,detail::lvalue_tag());
^
/usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here
{return final().insert_(x);}
^
/usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>::final_insert_' requested here
std::pair<final_node_type*,bool> p=this->final_insert_(x);
^
txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here
indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
```
```bash
In file included from test/fuzz/policy_estimator.cpp:9:
In file included from ./test/fuzz/util.h:27:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
Marker& operator=(const Marker&) = delete;
^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
__a.construct(__p, std::forward<_Args>(__args)...);
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
_Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::emplace_back<CTxMemPoolEntry>' requested here
{ emplace_back(std::move(__x)); }
^
test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
```
</p></details>
ACKs for top commit:
laanwj:
Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
vasild:
ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
Tree-SHA512: 0406dfcec180152d4f9ed07cbc2f406ad739b41f9c9cb38f8c75159c15d9d8a9a5c7526765966e40d695d265c178f6a80152e7edf82da344a65e55938dddb63d
9b85a5e2f7e003ca8621feaac9bdd304d19081b4 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511d8c43b2025a89bcd8295de755346a7 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4 🎰
ryanofsky:
Code review ACK 9b85a5e2f7e003ca8621feaac9bdd304d19081b4. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
## Issue being fixed or feature implemented
Platform (in the scope of Withdrawals) need to be aware if a tx isn't in
mempool when requesting status of a tx using RPC `gettxchainlocks`.
cc @markin-io
## What was done?
- mempool is passed to `GetTransaction` and saving the result for
checking latter.
- If the returned tx_ref is nullptr, then the RPC returns null for the
corresponding tx in the array.
Example:
`tx1` is mined and chainlocked, `tx2` is in mempool and `tx3` doesn't
exist.
The result now is:
`[
{
"height": 830,
"chainlock": false,
"mempool": true
},
{
"height": -1,
"chainlock": false,
"mempool": true
},
{
"height": -1,
"chainlock": false,
"mempool": false
}
]`
## How Has This Been Tested?
## Breaking Changes
## 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)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Platform in the scope of credit withdrawals, need a way to get the
status of an Asset Unlock by index.
## What was done?
A new RPC was created `getassetunlockchainlocks` that accepts Asset
Unlock indexes array as parameter and return corresponding status for
each index.
The possible outcomes per each index are:
- `chainlocked`: If the Asset Unlock index is mined on a Chainlocked
block.
- `mined`: If no Chainlock information is available, and the Asset
Unlock index is mined.
- `mempooled`: If the Asset Unlock index is in the mempool.
- `unknown`: If none of the above are valid.
Note: This RPC is whitelisted for the Platform RPC user.
## How Has This Been Tested?
Inserted on `feature_asset_locks.py` covering cases where Asset Unlock
txs are in mempool, mined and not present.
## Breaking Changes
no
## 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 _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: pasta <pasta@dashboost.org>
## What was done?
Add builder key for @knst
## How Has This Been Tested?
It is the same file with:
https://github.com/dashpay/guix.sigs/blob/master/builder-keys/knst.pgp
Checked a signature of guix.sigs for 20.0.2: `gpg --status-fd 1 --verify
20.0.2/knst/codesigned.SHA256SUMS.asc` - matched.
## 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
