2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
This adds a new descriptor with syntax e.g. tr(KEY,{S1,{{S2,S3},S4})
where KEY is a key expression for the internal key and S_i are
script expression for the leaves. They have to be organized in
nested {A,B} groups, with exactly two elements.
tr() only exists at the top level, and inside the script expressions
only pk() scripts are allowed for now.
aca0e5dcdb Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo)
b3e67f20a0 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
4c3a5dcbfc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
13bd8bb053 Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo)
4d8189f620 scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo)
0f53df47d5 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo)
716de29dd8 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo)
Pull request description:
This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls.
The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: #21789) instead of having it as a global variable (i.e. `gArgs`).
**Notes:**
* First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615274095) but not fully applied in #21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in #21244) This commit can be in a separate PR too.
* Other commits deal with removing of `GetDataDir(net_specific)` function.
* This was originally part of #21244 but it was [left]((https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-633779754)) for a follow up PR.
* I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing.
If you know about a better approach how to do this, please share it here.
ACKs for top commit:
hebasto:
ACK aca0e5dcdb
MarcoFalke:
re-ACK aca0e5dcdb👃
Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
e286cd0d7b net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
74bf850ac4 faster HexStr => 13% faster blockToJSON (Martin Ankerl)
Pull request description:
`std::string`'s push_back is rather slow because it needs to check & update the string size. For
`HexStr` the output string size is already easily know, so we can initially create the string with
the correct size and then just assign the data.
`HexStr` is heavily usd in `blockToJSON`, so this change is a noticeable benefit. Benchmark on an i7-8700 @3.2GHz:
* 71,315,461.00 ns/op master
* 62,842,490.00 ns/op this commit
So this little change makes `blockToJSON` about ~13% faster.
ACKs for top commit:
laanwj:
Code review ACK 74bf850ac4
theStack:
re-ACK 74bf850ac4
Tree-SHA512: fc99105123edc11f4e40ed77aea80cf7f32e49c53369aa364b38395dcb48575e15040b0489ed30d0fe857c032a04e225c33e9d95cdfa109a3cb5a6ec9a972415
fa340b8794 refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash (MarcoFalke)
fae33f98e6 Fix assumeutxo crash due to invalid base_blockhash (MarcoFalke)
fa5668bfb3 refactor: Use type-safe assumeutxo hash (MarcoFalke)
0000007709 refactor: Remove unused code (MarcoFalke)
faa921f787 move-only: Add util/hash_type (MarcoFalke)
Pull request description:
Starting with commit d6af06d68a, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.
Stack trace (copied from https://github.com/bitcoin/bitcoin/pull/21584#discussion_r612673879):
```
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff583c8b1 in __GI_abort () at abort.c:79
#2 0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89,
function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
#3 0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89,
function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
#4 0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
#5 0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
#6 0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
#7 0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
at validation.cpp:5422
#8 0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
#9 0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=...,
root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
#10 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
at test/validation_chainstatemanager_tests.cpp:262
ACKs for top commit:
laanwj:
Code review re-ACK fa340b8794
jamesob:
ACK fa340b8794 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))
Tree-SHA512: c2c4e66c1abfd400ef18a04f22fec1f302f1ff4d27a18050f492f688319deb4ccdd165ff792eee0a1f816e7b69fb64080662b79517ab669e3d26b9eb77802851
792be53d3e refactor: Replace std::bind with lambdas (Hennadii Stepanov)
a508f718f3 refactor: Use appropriate thread constructor (Hennadii Stepanov)
30e4448215 refactor: Make TraceThread a non-template free function (Hennadii Stepanov)
Pull request description:
This PR does not change behavior.
Its goal is to improve readability and maintainability of the code.
ACKs for top commit:
jnewbery:
utACK 792be53d3e
jonatack:
tACK 792be53d3e
MarcoFalke:
cr ACK 792be53d3e
Tree-SHA512: a03142f04f370f6bc02bd3ddfa870819b51740fcd028772241d68c84087f95a2d78207cbd5edb3f7c636fcf2d76192d9c59873f8f0af451d3b05c0cf9cf234df
This reverts commit eac6a3080d ("refactor:
Rework asmap Interpret to avoid ptrdiff_t"), because it is UB to form a
past-the-end iterator, even if it is never dereferenced.
Then fix the compiler warning in a different way:
Instead of comparing an uint32_t against a signed ptrdiff_t, just
promote both to a type that can represent both types.
Even though in this case the ptrdiff_t should never hold a negative
value, the overhead from promotion should be negligible.
916ab0195d remove unused class util::Ref and its unit test (Sebastian Falbesoner)
8dbb87a393 refactor: replace util::Ref by std::any (C++17) (Sebastian Falbesoner)
95cccf8a4b util: introduce helper AnyPtr to access std::any instances (Sebastian Falbesoner)
Pull request description:
As described in `util/ref.h`: "_This implements a small subset of the functionality in C++17's std::any class, and **can be dropped when the project updates to C++17**_". For accessing the contained object of a `std::any` instance, a helper template function `AnyPtr` is introduced (thanks to ryanofsky).
ACKs for top commit:
hebasto:
re-ACK 916ab0195d, with command
ryanofsky:
Code review ACK 916ab0195d. Changes since last review: rebase and replacing types with `auto`. I might have used `const auto*` and `auto*` instead of plain `auto` because I think the qualifiers are useful, but this is all good.
Tree-SHA512: fe2c3e4f5726f8ad40c61128339bb24ad11d2c261f71f7b934b1efe3e3279df14046452b0d9b566917ef61d5c7e0fd96ccbf35ff810357e305710f5002c27d47
40316a37cb test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77970 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a8 net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d01 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)
Pull request description:
Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.
Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in https://github.com/bitcoin/bitcoin/pull/21407.
ACKs for top commit:
practicalswift:
Tested ACK 40316a37cb
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb reviewed `git range-diff 01bb3afb 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
laanwj:
Code review ACK 40316a37cb
Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
fa4cebadcf util: Make Assume() usable as unary expression (MarcoFalke)
Pull request description:
Assume shouldn't behave different at the call site depending on build flags. Currently compilation fails if it is used as expression. Fix that by using the lambda approach from `Assert()` without the `assert()`.
ACKs for top commit:
jnewbery:
ACK fa4cebadcf
practicalswift:
cr ACK fa4cebadcf: patch looks correct and commit hash starts with `fa`
Tree-SHA512: 9ec9ac8d410cdaf5e4e28df571a89e3d23d38e05a7027bb726cae3da6e9314734277e5a218e9e090cc17e10db763da71052c229ad642077ca5824ee42022f3ed
52dd40a9fe test: add missing netaddress include headers (Jon Atack)
6f09c0f6b5 util: add missing braces and apply clang format to SplitHostPort() (Jon Atack)
2875a764f7 util: add ParseUInt16(), use it in SplitHostPort() (Jon Atack)
6423c8175f p2p, refactor: pass and use uint16_t CService::port as uint16_t (Jon Atack)
Pull request description:
As noticed during review today in https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584873708 of the upcoming I2P network support, `CService::port` is `uint16_t` but is passed around the codebase and into the ctors as `int`, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
ACKs for top commit:
practicalswift:
cr ACK 52dd40a9fe: patch looks correct
MarcoFalke:
cr ACK 52dd40a9fe
vasild:
ACK 52dd40a9fe
Tree-SHA512: 203c1cab3189a206c55ecada77b9548b810281cdc533252b8e3330ae0606b467731c75f730ce9deb07cbaab66facf97e1ffd2051084ff9077cba6750366b0432
We want `Get()` to always return the same value, otherwise it will look
like the `FuzzedSock` implementation itself is broken. So assign
`m_socket` a random number in the `FuzzedSock` constructor.
There is nothing to fuzz about the `Get()` and `Release()` methods, so
use the ones from the base class `Sock`.
`Reset()` is just setting our socket to `INVALID_SOCKET`. We don't want
to use the base `Reset()` because it will close `m_socket` and given
that our `m_socket` is just a random number it may end up closing a real
opened file descriptor if it coincides with our random `m_socket`.
7059e6d822 test: add a test to ensure RecvUntilTerminator() limit works (Vasil Dimov)
80a5a8ea2b i2p: limit the size of incoming messages (Vasil Dimov)
Pull request description:
Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.
In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
ACKs for top commit:
laanwj:
Re-ACK 7059e6d822
Tree-SHA512: 21f3f3468c765c726cdc877eae847cdb4dbe225d94c5bd1849bd752c9761fac881c554f16ea7a685ad40312159d554e126c481e21c5fd83a6d947060b920373d
Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.
In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
1a6323bdbe doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7e scripted-diff: remove MakeUnique<T>() (fanquake)
Pull request description:
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.
ACKs for top commit:
jnewbery:
utACK 1a6323bdbe
practicalswift:
cr ACK 1a6323bdbe: patch looks correct
ajtowns:
ACK 1a6323bdbe -- code review only
glozow:
ACK 1a6323bdbe looks correct
Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
e017a913d0 bitcoind: Add -daemonwait option to wait for initialization (Wladimir J. van der Laan)
c3e6fdee6d shutdown: Use RAII TokenPipe in shutdown (Wladimir J. van der Laan)
612f746a8f util: Add RAII TokenPipe (Wladimir J. van der Laan)
Pull request description:
This adds a `-daemonwait` flag that does the same as `-daemon` except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.
This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.
The use of the libc function `daemon()` is replaced by a custom implementation which is inspired by the [glibc implementation](https://github.com/lattera/glibc/blob/master/misc/daemon.c#L44), but which also creates a pipe from the child to the parent process for communication.
An additional advantage of having our own `daemon()` implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.
TODO:
- [x] Factor out `token_read` and `token_write` to an utility, and use them in `shutdown.cpp` as well—this is exactly the same kind of communication mechanism.
- [x] RAII-ify pipe endpoints.
- [x] Improve granularity of the `configure.ac` checks. This currently still checks for the function `daemon()` which makes no sense as it's not used. It should check for individual functions such as
`fork()` and `setsid()` etc—the former being required, the second optional.
- [-] ~~Signal propagation during initialization: if say, pressing Ctrl-C during `-daemonwait` it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free.~~ This is not necessary, see https://github.com/bitcoin/bitcoin/pull/21007#issuecomment-769007341.
Future:
- Consider if it makes sense to use this in the RPC tests (there would be no more need for "is RPC ready" polling loops). I think this is out of scope for this PR.
ACKs for top commit:
jonatack:
Tested ACK e017a913d0 checked change since previous review is move-only
Tree-SHA512: 53369b8ca2247e4cf3af8cb2cfd5b3399e8e0e3296423d64be987004758162a7ddc1287b01a92d7692328edcb2da4cf05d279b1b4ef61a665b71440ab6a6dbe2
