fa7fea3654203bf7e7bd504589dd564af7fc749d refactor: Remove mempool global from net (MarcoFalke)
Pull request description:
To increase modularisation and simplify testing, remove the mempool global from net in favour of a mempool member.
This is done in the same way it was done for the connection manager global.
ACKs for top commit:
jnewbery:
code review ACK fa7fea3654203bf7e7bd504589dd564af7fc749d
Tree-SHA512: 0e3e1eefa8d6e46367bc6991d5f36c636b15ae4a3bda99b6fe6715db3240771c3d87943c6eb257d69f31929fa2f1d0973e14fc9d1353a27551dbe746eae36857
fb15bfd99e6ed705cb6f7c4dcd1b2a6bc68be79e Fix nit in getblockchaininfo (Steven Roose)
Pull request description:
Noticed that the statistics are not always shown.
ACKs for top commit:
laanwj:
ACK fb15bfd99e6ed705cb6f7c4dcd1b2a6bc68be79e
promag:
ACK fb15bfd99e6ed705cb6f7c4dcd1b2a6bc68be79e.
Tree-SHA512: bccbfdff03107d14967f6530eec0bcada7ba8eb16c61b829119533a73f2ead742a0da6a473b7962b15e25cd685c8f155506ab16d4a95b20352d3fd1b4b0164a3
7df0cf719fecf1b8d09801295db8bd2137bfe033 Replace remaining literals BTC with CURRENCY_UNIT (Daniel Kraft)
Pull request description:
This replaces one remaining instance of the literal `"BTC"` string with the `CURRENCY_UNIT` constant, as is done in most of the codebase already.
After this change, no instance of literal `"BTC"` remains anywhere in the RPC help texts.
ACKs for top commit:
MarcoFalke:
ACK 7df0cf719fecf1b8d09801295db8bd2137bfe033
laanwj:
ACK 7df0cf719fecf1b8d09801295db8bd2137bfe033
Tree-SHA512: 7f7d52b366e084c93a7d6a3c45b1bbfc4f4f50bca6956594077e6d46295977c8cc18499232878869869c73a8ab9a1c41245029ae7425a87cec2ccb0cb52eea13
686c5456f2fcf7e301907223d16a85f7eb378c6c Fix missing header in sync.h (João Barbosa)
Pull request description:
`std::string` is referenced in `sync.h` but the relevant header is not explicitly included as required by current guideline. Furthermore on osx 10.14.6 with clang-900.0.31 the following error occurs:
```
In file included from threadinterrupt.cpp:6:
In file included from ./threadinterrupt.h:8:
./sync.h:206:21: error: implicit instantiation of undefined template 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >'
std::string lockname;
```
ACKs for top commit:
practicalswift:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
laanwj:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
Tree-SHA512: 7c1acdfa5b0dd148d1114e14c9450d5907006e63e1a04e82ed8a1e29757925476e6f8ef6024b0c6d1bb596623115209ad580d5035be1e4785337bd01b738c9f2
2a1b85f3c54fdaa8d13aeaf7e1acab1afb98fd97 tx: Bump transifex slug to 020x (Wladimir J. van der Laan)
82dd8860bbe81567e4afb033d9026dc0e69b1574 qt: Periodical translations update (Wladimir J. van der Laan)
Pull request description:
Need to merge this so that translations for 0.20 can start.
Top commit has no ACKs.
Tree-SHA512: c78307eea3130b9bbc301f23790340e4c86a3f3877ab308bf67d9e1c9e977f7dda4b2cd83b96d9398b01dce616a6c962b98115123628bba35170eb0f301c4dd8
This change adds to the BITCOIN_QT_CONFIGURE script ability to use
pkg-config for MinGW. All of the non-pkg-config paths are removed as
needless.
If depends is built with DEBUG=1 the configure script fails to pickup
Qt:
- for macOS host (similar, but not the same as issue 16391)
- for Windows host (regression)
fa192952507dbe5f405abffce38b3556923ba271 test: Bump timeouts to avoid valgrind failures (MarcoFalke)
Pull request description:
This should fix ci timeout issues such as:
* https://travis-ci.org/github/bitcoin/bitcoin/jobs/661946972#L3109
* https://travis-ci.org/github/bitcoin/bitcoin/jobs/662521570#L4922
ACKs for top commit:
practicalswift:
ACK fa192952507dbe5f405abffce38b3556923ba271
Tree-SHA512: 150f804957575033a83fd47c68fd6adff2c11b110ec5803bd77f121256349805b595c39a3a5047738ec538d082ee38cebbcb6792c787ef22f56b8dd0d9618c1f
e6e622e5a0e22c2ac1b50b96af818e412d67ac54 Implement O(1) OP_IF/NOTIF/ELSE/ENDIF logic (Pieter Wuille)
d0e8f4d5d8ddaccb37f98b7989fb944081e41ab8 [refactor] interpreter: define interface for vfExec (Anthony Towns)
89fb241c54fc85befacfa3703d8e21bf3b8a76eb Benchmark script verification with 100 nested IFs (Pieter Wuille)
Pull request description:
While investigating what mechanisms are possible to maximize the per-opcode verification cost of scripts, I noticed that the logic for determining whether a particular opcode is to be executed is O(n) in the nesting depth. This issue was also pointed out by Sergio Demian Lerner in https://bitslog.wordpress.com/2017/04/17/new-quadratic-delays-in-bitcoin-scripts/, and this PR implements a variant of the O(1) algorithm suggested there.
This is not a problem currently, because even with a nesting depth of 100 (the maximum possible right now due to the 201 ops limit), the slowdown caused by this on my machine is around 70 ns per opcode (or 0.25 s per block) at worst, far lower than what is possible with other opcodes.
This PR mostly serves as a proof of concept that it's possible to avoid it, which may be relevant in discussions around increasing the opcode limits in future script versions. Without it, the execution time of scripts can grow quadratically with the nesting depth, which very quickly becomes unreasonable.
This improves upon #14245 by completely removing the `vfExec` vector.
ACKs for top commit:
jnewbery:
Code review ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
MarcoFalke:
ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54 🐴
fjahr:
ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
ajtowns:
ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
laanwj:
concept and code review ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
jonatack:
ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54 code review, build, benches, fuzzing
Tree-SHA512: 1dcfac3411ff04773de461959298a177f951cb5f706caa2734073bcec62224d7cd103767cfeef85cd129813e70c14c74fa8f1e38e4da70ec38a0f615aab1f7f7
This replaces one remaining instance of the literal "BTC" string with
the CURRENCY_UNIT constant, as is done in most of the codebase already.
The other remaining instance (which is just part of a log message and thus
not really user-visible) is just removed.
After this change, no instance of literal "BTC" remains anywhere in the
non-Qt and non-test codebase.
09e25071f40c564af08a1386c39c4f2d8eb484b6 Cache parent xpub inside of BIP32PubkeyProvider (Andrew Chow)
deb791c7ba057a3765d09b12bf3e55547a5298e4 Only cache xpubs that have a hardened last step (Andrew Chow)
f76733eda5f4c161e9eb47c74b949582ab8f448a Cache the immediate derivation parent xpub (Andrew Chow)
58f54b686f663e4c46a2cf7a64560409007c7eb3 Add DescriptorCache* read_cache and DescriptorCache* write_cache to Expand and GetPubKey (Andrew Chow)
66c2cadc91d26074b89e5ada68350b5c8676efac Rename BIP32PubkeyProvider.m_extkey to m_root_extkey (Andrew Chow)
df55d44d0de2174ba74ed3a28bef5e83b0a51b47 Track the index of the key expression in PubkeyProvider (Andrew Chow)
474ea3b927ddc67e64ae78e08c20c9264817e84d Introduce DescriptorCache struct which caches xpubs (Andrew Chow)
Pull request description:
Improves the descriptor cache by changing it from a `std::vector<unsigned char>` to a newly introduced `DescriptorCache` class. Instead of serializing pubkeys and whatever else we would want to cache in a way that may not be backwards compatible, we instead create a `DescriptorCache` object and populate it. This object contains only an xpub cache. Since the only `PubkeyProvider` that used the cache is the `BIP32PubkeyProvider` we just have it store the xpubs instead of the pubkeys. This allows us to have both the parent xpub and the child xpubs in the same container. The map is keyed by `KeyOriginInfo`.
Sine we are caching `CExtPubKey`s in `DescriptorCache`, `BIP32PubKeyProviders` can use the cached parent xpubs to derive the children if unhardened derivation is used in the last step. This also means that we can still derive the keys for a `BIP32PubkeyProvider` that has hardened derivation steps. When combined with descriptor wallets, this should allow us to be able to import a descriptor with an `xprv` and hardened steps and still be able to derive from it. In that sense, this is an alternative to #18163
To test that this works, the tests have been updated to do an additional `Expand` at the `i + 1` position. This expansion is not cached. We then do an `ExpandFromCache` at `i + 1` and use the cache that was produced by the expansion at `i`. This way, we won't have the child xpubs for `i + 1` but we will have the parent xpubs. So this checks whether the parent xpubs are being stored and can be used to derive the child keys. Descriptors that have a hardened last step are skipped for this part of the test because that will always require private keys.
ACKs for top commit:
instagibbs:
code review re-re-ACK 09e25071f4
Sjors:
re-ACK 09e25071f40c564af08a1386c39c4f2d8eb484b6
Tree-SHA512: 95c8d0092274cdf115ce39f6d49dec767679abf3758d5b9e418afc308deca9dc6f67167980195bcc036cd9c09890bbbb39ec1dacffbfacdc03efd72a7e23b276
c8e24ddce31a8de6255b23c19d958c1cd44a8847 [REFACTOR] Abstract out script execution out of VerifyWitnessProgram() (Pieter Wuille)
Pull request description:
This is a refactoring cherry-picked out of #17977. As it touches consensus code, I don't think this would ordinarily meet the bar for review cost vs benefit. However, it simplifies the changes for Taproot significantly, and if it's going to be necessitated by inclusion of that code, I may as well give it some additional attention by PRing it independently.
ACKs for top commit:
fjahr:
Re-ACK c8e24ddce31a8de6255b23c19d958c1cd44a8847
theStack:
re-ACK c8e24ddce3
Empact:
Code Review Re-ACK c8e24ddce3
ajtowns:
ACK c8e24ddce31a8de6255b23c19d958c1cd44a8847
jnewbery:
ACK c8e24ddce31a8de6255b23c19d958c1cd44a8847
jonatack:
ACK c8e24dd
Tree-SHA512: 96c2aa5d2f9c7c802bcc008f5cde55b1dfedfaf42e34101331e6c0d594acdf6437661102dc939718f0877c20451336855dfbaa8aa8f57d9e722a7fa7329e3a46
a33cffbeabcc42137c4a66aa19b7dd1d300e6d73 util: HelpExampleRpc formatting fixup (Jon Atack)
Pull request description:
Minor visual fixup of the HelpExampleRpc template; conforms to the JSON-RPC spec as per https://www.jsonrpc.org/specification#examples. (I'm... somewhat embarassed to open such a minor change, but this is what is shown in all the CLI/RPC help docs.)
ACKs for top commit:
laanwj:
ACK a33cffbeabcc42137c4a66aa19b7dd1d300e6d73
Tree-SHA512: 8f1dee080c224742fff60a33fec6f5fb1d59c9fa51f3f2a67bf2e1837dbfa25f12a69e34518936588940013b0e61f55378b4f1a571c47c3cb081ca5b245e1091
faf6f156ffd1a8ed1aed047428d791a8c13c162b test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
Pull request description:
The wallet rebroadcast functionality learns about new blocks via the validation interface queue. To avoid test failures such as https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31119387#L466 , we can sync with the queue before advancing the test.
ACKs for top commit:
jonatack:
ACK faf6f156 this makes sense; the fix was previously added to mempool_persist.py and wallet_zapwallettxes.py in #12217 and to wallet_balance.py in #16302. It is also used in src/test/validation_block_tests.cpp (processnewblock_signals_ordering) and src/bench/wallet_balance.cpp.
Tree-SHA512: d72fd4b597b669d8111007902b523e946712913cd6eea6f9a695b0f04ecbe2321d05019873af999a95b9e0aa0f5c140a17109b37503723e40c9eab24ec358eb7
fa2cf85e6f366d62ebf173bf0bd51af45806afe1 test: Fix race in p2p_segwit (MarcoFalke)
Pull request description:
Fixes #11696
Top commit has no ACKs.
Tree-SHA512: 09de07ea26236547586f5c373a0df2b68d84af5cfa8f40bd2ca9f951fc083c5f4b8a472a60668d99118bbd9f3942ec3d6a34f05944d47345acca41c95475bb27
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.
0ae42a16c766a7ecb8711bfad6f22b8581ea0258 guix: Remove now-unnecessary gcc make flag (Carl Dong)
Pull request description:
```
Previously, Guix would produce a gcc which did not know to use the SSP
function from glibc, and required a gcc make flag for it to do so, in my
attempt to fix it upstream I realized that this is no longer the case.
This can be verified by performing a Guix build and doing
readelf -s ... | grep __stack_chk
to check that symbols are coming from glibc, and doing
readelf -d ... | grep NEEDED | grep ssp
to see that libssp.so is not being depended on
```
ACKs for top commit:
fanquake:
ACK 0ae42a16c766a7ecb8711bfad6f22b8581ea0258 - ran a Guix build (hashes below) and checked all the linux binaries:
Tree-SHA512: 701b91e7c323b12a29af9539cb2656d10ce0a93af573a02e57f0b7fea05a6e1819798536eadb24d0a17e7f35b503f5e863fee5e7409db1b8a3973c4375e49d4e
NotifyEntryAdded never had any subscribers so can be removed.
Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.
The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved
callback to be notified of transactions removed for conflict. Since
PerBlockConnectTrace no longer tracks conflicted transactions,
ConnectTrace no longer requires these notifications.
The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.
Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
Previously, Guix would produce a gcc which did not know to use the SSP
function from glibc, and required a gcc make flag for it to do so, in my
attempt to fix it upstream I realized that this is no longer the case.
This can be verified by performing a Guix build and doing
readelf -s ... | grep __stack_chk
to check that symbols are coming from glibc, and doing
readelf -d ... | grep NEEDED | grep ssp
to see that libssp.so is not being depended on
111880aaf7e12a12f0797f1b19673e3d96328edd [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley)
Pull request description:
This adds light functional coverage to estimaterawfee - a subset of
the testing applied to estimatesmartfee, and argument validation
testing to both estimaterawfee and estimatesmartfee.
One valid estimatesmartfee signature test is commented out because it
fails currently.
Extracted from #12940
Top commit has no ACKs.
Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
