d40550d725 scripted-diff: remove duplicate categories from LogPrint output (Jon Atack)
Pull request description:
This is the first commit from #25203.
- Scripted-diff: de-duplicate logging category output for the tor, i2p, net, zmq, and prune messages (e.g. where I found duplicates), as these category prefixes are now printed automatically since #24464
examples before
```
[tor] tor: Successfully connected!
[i2p] I2P: Creating SAM session with 127.0.0.1:7656
[zmq] zmq: Initialize notification interface
[net] net: enabling extra block-relay-only peers
```
after
```
[tor] Successfully connected!
[i2p] Creating SAM session with 127.0.0.1:7656
[zmq] Initialize notification interface
[net] enabling extra block-relay-only peers
```
ACKs for top commit:
klementtan:
crACK d40550d725
MarcoFalke:
cr ACK d40550d725
Tree-SHA512: 63b799f2f899f0597981dd1acb91ef4439cd00b257a9eb19d67c4ce2c4dc72a95ac5761cb78f2a19090a10be74f23ea1db6929ed942ba0d008b4be563f0d5e7e
fa4068b4e2 Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
f26a496dfd test: clean up all-lint.py (Martin Leitner-Ankerl)
64d72c4c87 test: rename lint-all.py to all-lint.py (Martin Leitner-Ankerl)
Pull request description:
When running `./test/lint/lint-all.py`, the script runs all tests but
also calls itself because the comparison with `__file__` doesn't work.
Comparing resolved paths gives reliable comparison, and lint-all.py doesn't call itself any more
ACKs for top commit:
laanwj:
Code review ACK f26a496dfd
Tree-SHA512: b44abdd685f7b48a6a9f48e96d97138b635c31c1c7ab543cb5636b5f49690ccd56fa6fec01ae7fcc16af01a613372ee77632f70c32059919b373aa8051953791
e593ae07c4 Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block (Luke Dashjr)
Pull request description:
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.
~~(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)~~
ACKs for top commit:
fjahr:
utACK e593ae07c4
ryanofsky:
Code review ACK e593ae07c4. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!
Tree-SHA512: c2d511df80682d57260aae8af1665f9d7eaed16448f185f4c9f23c78fa9b8289a02053da7a0b83643fef57610d601ea63b59ff39661a51f4827f1eb27cc30594
3a9b9bb38e test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes#25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e
laanwj:
Code review ACK 3a9b9bb38e
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
c47944f4e9 ci: Reuse some configure options in "ARM64 Android APK" task (Hennadii Stepanov)
7739438811 ci, android: Update NDK up to r23c (Hennadii Stepanov)
ca0c3e5077 ci, android: Update Command-line Tools from 2.1 up to 7.0 (Hennadii Stepanov)
8790da3c1e ci: Drop unneeded packages in "ARM64 Android APK" task (Hennadii Stepanov)
Pull request description:
This PR improves the "ARM64 Android APK" CI task in the following ways:
- dropped packages that are not required to be installed
- updated Android Command-line Tools and Android NDK to make the CI environment closer to the default one, which is provided by Android Studio
ACKs for top commit:
icota:
utACK c47944f4e9
Tree-SHA512: 45f5aba41007a502ae90333272370fd559c48a27d573896c449b3e436c5cf2b6440408381e4d20eb53104426ade26d3a9014c09dcdf3257ec897a537095efa4f
d1b7bcbca2 qt: Drop no longer supported Android architecture (Hennadii Stepanov)
Pull request description:
The `i686-linux-android` arch support has been dropped since bitcoin/bitcoin#23744.
ACKs for top commit:
katesalazar:
ACK d1b7bcbca2
icota:
utACK d1b7bcbca2
prusnak:
Approach ACK d1b7bcbca2
Tree-SHA512: 13689ec8c63c92b9a52a3c25edc35536b8e51ff583f57c45b168515f928d020d6bb85d03db9efd8d5efd57b944dfd313a89f5ff8a52f99982ccc8d9671f6e7a9
f3a50c9dfe miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe5163dc miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9d8d miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e4249d miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6c59 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf7bc fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d5077b fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0ac0 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f12c miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064dc14 miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3882 miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89fd1 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c662c0 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db0f0 miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)
Pull request description:
The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to #24148 but i think there are enough of them to be worth a followup PR on its own.
Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in #24149 that will be contained in this file too.
The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after #24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.
This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)
ACKs for top commit:
sipa:
utACK f3a50c9dfe (with the caveat that a lot of it is my own code)
sanket1729:
code review ACK f3a50c9dfe. Did not review the fuzz tests.
Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb
d87784ac87 kernel: SanityChecks: Return an error struct (Carl Dong)
265d6393bf Move init::SanityCheck to kernel::SanityCheck (Carl Dong)
fed085a1a4 init: Initialize globals with kernel::Context's life (Carl Dong)
7d03feef81 kernel: Introduce empty and unused kernel::Context (Carl Dong)
eeb4fc20c5 test: Use Set/UnsetGlobals in BasicTestingSetup (Carl Dong)
Pull request description:
The full `init/common.cpp` is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren't necessary for libbitcoinkernel so we only extract the portion that is necessary (namely `init::{Set,Unset}Globals()`.
ACKs for top commit:
theuni:
ACK d87784ac87
vasild:
ACK d87784ac87
Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last
block pruned was returned, subject to a bug if there were blocks left unpruned
due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new
bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it
makes more sense to fix the behaviour to match the documentation.
3a171f742c logging: fix logging empty threadname (klementtan)
Pull request description:
Currently, `leveldb` background thread does not have a thread name and as a result, an empty thread name is logged.
This PR fixes this by logging thread name as `"unknown"` if the thread name is empty
On master:
```txt
2022-06-02T14:30:38Z [] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
On this PR:
```txt
2022-06-02T14:30:38Z [unknown] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
ACKs for top commit:
laanwj:
Code review ACK 3a171f742c
hebasto:
ACK 3a171f742c
Tree-SHA512: 0af0fa5c4ddd3640c6dab9595fe9d97f74d0e0f4b41287a6630cf8ac5a21240250e0659ec4ac5a561e888d522f5304bf627104de2aba0fd0a86c1222de0897c2
48262a00f5 Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bb Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f5
sipa:
ACK 48262a00f5
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
1bace0cfee test: check `replaceable` mismatch error in `createrawtransaction` RPC (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the createrawtransaction RPC, in the case that the `replaceable` parameter is set, but the passed transaction doesn't signal RBF (i.e. no input's nSequence is < 0xffffffe):
1c7ef0abd1/src/rpc/rawtransaction_util.cpp (L135-L137)
ACKs for top commit:
laanwj:
Code review ACK 1bace0cfee
brunoerg:
crACK 1bace0cfee
furszy:
Code review ACK 1bace0cf
Tree-SHA512: e6390401c8026c782643e3de7be56ea1745736b7e4c8886186d75c353c301b57afdabb631b9d8e2770386e4d7a59ac5fba1f380b9a5a21296512ca4515b35baa
d2f8f1b307 use testing setup mempool in ComplexMemPool bench (glozow)
aecc332a71 create and use mempool transactions using real coins in MempoolCheck (glozow)
2118750631 [test util] to populate mempool with random transactions/packages (glozow)
5374dfc4e3 [test util] use -checkmempool for TestingSetup mempool check ratio (glozow)
d7d9c7b266 [test util] add chain name to TestChain100Setup ctor (glozow)
Pull request description:
Fixes#24634 by using the `testing_setup`'s actual mempool rather than a locally-declared mempool for running `check()`.
Also creates a test utility for populating the mempool with a bunch of random transactions. I imagine this could be useful in other places as well; it was necessary here because we needed the mempool to contain transactions *spending coins available in the current chainstate*. The existing `CreateOrderedCoins()` is insufficient because it creates coins out of thin air.
Also implements the separate suggestion to use the `TestingSetup` mempool in `ComplexMemPool` bench.
ACKs for top commit:
laanwj:
Code review ACK d2f8f1b307
Tree-SHA512: 44ab5a9e55b126b5a5bc33f05fbad1380b9c43c84736c7cf487be025e0e3f5d75216ccf5a3088b0935da817e3dacfba99d2885f75bcb6e7eaa24cd20a82c24c8
a4741bd8d4 kernel: pass params to BlockManager rather than using a global (Cory Fields)
Pull request description:
In a discussion today, dongcarl and I realized that is the only usage of the global `Params()` left in the kernel code.
We can use the readily available reference in `ChainstateManager` instead.
Note: There are still some uses of `BaseParams` in the kernel, so it doesn't make sense to rearrange the definitions quite yet. Once those are gone we can split the globals into new files.
ACKs for top commit:
MarcoFalke:
cr ACK a4741bd8d4
laanwj:
Code review ACK a4741bd8d4
Tree-SHA512: bfcc0c35e6c23689e968ccc96ceda39dd5a47fe94fbe617902110fe5865c30a40ea614bcfd4b4a2c846d2e84340aa8973e70b0938786af0fecfa3e6016d7fcad
...instead of explicitly calling init::{Set,Unset}Globals.
Cool thing about this is that in both the testing and bitcoin-chainstate
codepaths, we no longer need to explicitly unset globals. The
kernel::Context goes out of scope and the globals are unset
"automatically".
Also construct kernel::Context outside of AppInitSanityChecks()
fa72e0ba15 Use designated initializers (MarcoFalke)
Pull request description:
Designated initializers are supported since gcc 4.7 (Our minimum required is 8) and clang 3 (Our minimum required is 7). They work out of the box with C++17, and only msvc requires the C++20 flag to be set. I don't expect any of our msvc users will run into issues due to this. See also https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2022/bitcoin-core-dev.2022-03-10-19.00.log.html#l-114
ACKs for top commit:
kristapsk:
ACK fa72e0ba15
hebasto:
ACK fa72e0ba15
Tree-SHA512: a198e9addd9af69262a7e79ae4377b55697c8dfe768b8b3d444526544b1d1f85df46f0ae81b7541bf2f73e5868fb944b159e5bf234303c7b8b9d778afb0b2840
fafaad98f7 test: Set maxfeerate=0 in MiniWallet sendrawtransaction() (MacroFake)
Pull request description:
It should be safe to set, because MiniWallet will only ever deal with test transactions, so loss-of-funds is not a reason to keep the feerate check.
It is beneficial to set, as it makes tests less verbose to write. Also, it may speed up tests, as the fee-check can be skipped: https://github.com/bitcoin/bitcoin/pull/25087#discussion_r877246529
ACKs for top commit:
michaelfolkson:
ACK fafaad98f7
theStack:
Code-review ACK fafaad98f7
Tree-SHA512: 94c5c163595207a295c7b21f0127d669a9307f6f8b1de5e043d43c52a6714076e2fdce65f2644308a2b90c679642c94f771dab1ff8bc5c0c8b1f5013324b3902
025c6ca509 Squashed 'src/univalue/' changes from 6c19d050a9..de4f73ddca (MacroFake)
9b50a309ff refactor: Replace get_int by getInt<int> alias (MacroFake)
e4e8186ab4 refactor: Explicitly convert atomic<int> to int (João Barbosa)
Pull request description:
This bumps the univalue subtree and changes two lines of our code. Apart from the get_int -> getInt change, this is mostly a rebase of https://github.com/bitcoin/bitcoin/pull/15975, which was closed back then.
However, given the numerous UniValue copy bugs and performance regressions in the past years, I think it makes sense to finally go through with the changes and disable potentially expensive implicit UniValue copies, which may cause OOM.
The changes here are not strictly required for that, but make future changes less verbose and easier to review.
ACKs for top commit:
laanwj:
Code review ACK fa0cc61b7f
fanquake:
ACK fa0cc61b7f
Tree-SHA512: 9ab9e371e6a745a80c441e99fb9cd407602a8066df883135e0ea7eced7b0c6ef0e9bc88f1d99a2b4804128d636727229f44d72b5615dbf2d70da4af63fa6adec
46a890960e build: pass bdb cppflags only where needed (fanquake)
Pull request description:
Move bdb cppflags out of the catch-all `BITCOIN_INCLUDES`, and pass them
only where they are needed, which is in libbitcoin_node/wallet and the tests.
ACKs for top commit:
hebasto:
ACK 46a890960e
Tree-SHA512: ac639bf88be1c21c5205bb14c983330c72c37f17ec4c15a746a86c54dd83853e3d75802eff48c4647484b07c1bae943ca2c3d61d2a8b0bed07d9c5b4a71dcec4
1da5e45725 test: use MiniWallet for feature_dbcrash.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (feature_dbcrash.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in https://github.com/bitcoin/bitcoin/issues/20078.
ACKs for top commit:
laanwj:
Code review ACK 1da5e45725
brunoerg:
crACK 1da5e45725
Tree-SHA512: 75ee9a32fd1451254004797d695d18032bd0fcb66ebd88cf737e147e43812525f6e884ec05fcc4f76f566dc71174c8ed7347bcdce16567db6511746ae64cead0
52a797bfe5 doc: Avoid ADL for function calls (Hennadii Stepanov)
Pull request description:
It happened two times recently, when [ADL](https://en.cppreference.com/w/cpp/language/adl) popped up unexpectedly and brought some confusion:
- https://github.com/bitcoin/bitcoin/pull/24338/files#r805989994
> Any idea why this even compiles?
- https://www.erisian.com.au/bitcoin-core-dev/log-2022-02-18.html#l-51:
> 2022-02-18T03:24:14 \<dongcarl\> Does anyone know why this compiles? 6d3d2caa37
> 2022-02-18T03:24:14 \<dongcarl\> GetUTXOStatsWithHasher and MakeUTXOHasher are both in the `kernel::` namespace and I never added a `using` declaration on top...
> 2022-02-18T03:25:53 \<sipa\> https://en.cppreference.com/w/cpp/language/adl ?
Let's document our intention to avoid similar cases in the future.
ACKs for top commit:
laanwj:
Anyhow, ACK 52a797bfe5, there is no need to hold merge up on this, documenting it is a step forward.
Tree-SHA512: f52688b5d8f6130302185206ec6ea4731b099a75294ea2d477901a52d6d58473e3427e658aea408c140c2824c37a0399ec7376aded2a91197895ea52d51f0018
fa2d226ac9 doc: Explain squashing with merge commits (MacroFake)
Pull request description:
This avoids having to explain it in each thread
ACKs for top commit:
laanwj:
ACK fa2d226ac9
Tree-SHA512: e1533ee7c0ab0101c78aaebed97dc889b5eb941cf4c2dfbabbb5f0ec1bb7b1313a1a2e2405235d68c761f039373cebac67ce691a72c820a9252429d50c1ac7d5
7d0f67a0d5 test: check pre-segwit peer error in `getblockfrompeer` RPC (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `getblockfrompeer` RPC, in the case that a block is tried to be fetched from a pre-segwit peer (i.e. a peer that doesn't signal the service bit `NODE_WITNESS`):
d4d9daff7a/src/net_processing.cpp (L1564-L1565)
ACKs for top commit:
MarcoFalke:
cr ACK 7d0f67a0d5
Tree-SHA512: bc330820686fe45577e7a53d66e2a0b339ee3ca4ef348ba3cab0a78ed891e47b3651cadf3c6c3c35d1e9a95779df010322c12d37b36700e828f6064ae35842fd
...instead of calling initialization functions directly and having to
keep around a ECCVerifyHandle member variable.
This makes the initialization codepath of our tests more closely
resemble those of AppInitMain and potentially eases the review of
subsequent commit removing init::{Set,Unset}Globals.
[META] In a future commit, we will introduce a kernel::Context which
calls init::{Set,Unset}Globals in its ctor and dtor. It will be
owned by node::NodeContext, so in the end, this patchset won't
have made the previously local ECCVerifyHandle global.
