a0efe529e4 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)
Pull request description:
After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.
ACKs for top commit:
jamesob:
ACK a0efe529e4
Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
0f95247246 Integrate univalue into our buildsystem (Cory Fields)
9b49ed656f Squashed 'src/univalue/' changes from 98fadc0909..a44caf65fe (fanquake)
Pull request description:
This PR more tightly integrates building Univalue into our build system. This follows the same approach we use for [LevelDB](https://github.com/bitcoin-core/leveldb/), ([`Makefile.leveldb.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.leveldb.include)), and [CRC32C](https://github.com/bitcoin-core/crc32c) ([`Makefile.crc32c.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include)), and will be the same approach we use for [minisketch](https://github.com/sipa/minisketch); see #23114.
This approach yields a number of benefits, including:
* Faster configuration due to one less subconfigure being run during `./configure` i.e 22s with this PR vs 26s
* Faster autoconf i.e 13s with this PR vs 17s
* Improved caching
* No more issues with compiler flags i.e https://github.com/bitcoin/bitcoin/pull/12467
* More direct control means we can build exactly the objects we want
There might be one argument against making this change, which is that builders should have the option to use "proper shared/system libraries". However, I think that falls down for a few reasons. The first being that we already don't support building with a number of system libraries (secp256k1, leveldb, crc32c); some for good reason. Univalue is really the odd one out at the moment.
Note that the only fork of Core I'm aware of, that actively patches in support for using system libs, also explicitly marks them as ["DANGEROUS"](a886811721/configure.ac (L1430)) and ["NOT SUPPORTED"](a886811721/configure.ac (L1312)). So it would seem they exist more to satisfy a distro requirement, as opposed to something that anyone should, or would actually use in practice.
PRs like #22412 highlight the "issue" with us operating with our own Univalue fork, where we actively fix bugs, and make improvements, when upstream (https://github.com/jgarzik/univalue) may not be taking those improvements, and by all accounts, is not currently actively maintained. Bitcoin Core should not be hamstrung into not being able to fix bugs in a library, and/or have to litter our source with "workarounds", i.e #22412, for bugs we've already fixed, based on the fact that an upstream project is not actively being maintained. Allowing builders to use system libs is really only exacerbating this problem, with little benefit to our project. Bitcoin Core is not quite like your average piece of distro packaged software.
There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.
ACKs for top commit:
dongcarl:
ACK 0f95247246 less my comment above, always nice to have an include-able `sources.mk` which makes integration easier.
theuni:
ACK 0f95247246. Thanks fanquake for keeping this going.
Tree-SHA512: a7f2e41ee7cba06ae72388638e86b264eca1b9a8b81c15d1d7b45df960c88c3b91578b4ade020f8cc61d75cf8d16914575f9a78fa4cef9c12be63504ed804b99
5c34507ecb core_write: Rename calculate_fee to have_undo for clarity (fyquah)
8edf6204a8 release-notes: Add release note about getblock verbosity level 3. (fyquah)
459104b2aa rest: Add test for prevout fields in getblock (fyquah)
4330af6f72 rpc: Add test for level 3 verbosity getblock rpc call. (fyquah)
51dbc167e9 rpc: Add level 3 verbosity to getblock RPC call. (fyquah)
3cc95345ca rpc: Replace boolean argument for tx details with enum class. (fyquah)
Pull request description:
Author of #21245 expressed [time issues](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-902332088) in the original PR. Given that #21245 has received a lot of review*, I have decided to open this new pull request with [modifications required to get ACK from luke-jr ](https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-905150806) and a few nits of mine.
### Original PR description
> Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing `/rest/block` API by adding a `prevout` fields to tx inputs. This is mentioned in the change to the release notes.
>
> I added some functional tests that
>
> * checks that the RPC call still works when TxUndo can't be found
>
> * Doesn't display the "value" or "scriptPubKey" of the previous output when at a lower verbosity level
>
>
> This "completes" the issue #18771
### Possible improvements
* b0bf4f255f - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See https://github.com/bitcoin/bitcoin/pull/21245#issuecomment-894853784 for more context.
### Examples
Examples of the `getblock` output with various verbose levels. Note that `000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5` contains only 2 transactions.
#### Verbose level 0
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0
```
##### Verbose level 1
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1
```
##### Verbose level 2
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2
```
##### Verbose level 3
```bash
./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3
```
#### REST
```bash
curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json
```
<sub>* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.</sub>
Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message.
ACKs for top commit:
0xB10C:
ACK 5c34507ecb
meshcollider:
utACK 5c34507ecb
theStack:
ACK 5c34507ecb👘
promag:
Concept ACK 5c34507ecb
Tree-SHA512: bbff120d8fd76e617b723b102b0c606e0d8eb27f21c631d5f4cdab0892137c4bc7c65b1df144993405f942c91be47a26e80480102af55bff22621c19f518aea3
fa44b071fd test: Remove unused node from mining_prioritisetransaction (MarcoFalke)
Pull request description:
No need to make the test slower with an unused node
ACKs for top commit:
0xB10C:
Code Review ACK fa44b071fd
practicalswift:
cr ACK fa44b071fd
brunoerg:
tACK fa44b071fd
Tree-SHA512: fd49d3ac5ead3693e6d0df8000a4dca20521cdd7d9041a6de289a0fb48fb28f8d3e2c36c8e3510ad0da9cdb0abacbdda83edb86c9cb94a221c800ebdfe29021c
ffdd94d753 test: Fix wallet_multisig_descriptor_psbt.py (Hennadii Stepanov)
Pull request description:
The `wallet_multisig_descriptor_psbt.py`, which was introduced in the recent bitcoin/bitcoin#22067, has a merge conflict with the previously merged bitcoin/bitcoin#23207.
Fixed in this PR.
ACKs for top commit:
mjdietzx:
Tested ACK ffdd94d
S3RK:
ACK ffdd94d
Tree-SHA512: e8871aeebbe119e22347de19f62b4524e191885d66f94af802a33793dfa413790901fd54aeea1ab3d1b1487cb457e8a58b0aef19d0dc78b12a583646ba4af67e
53c9fa9e62 tracing: drop block_connected hash.toString() arg (0xb10c)
Pull request description:
The tracepoint `validation:block_connected` was introduced in #22006.
The first argument was the hash of the connected block as a pointer
to a C-like String. The last argument passed the hash of the
connected block as a pointer to 32 bytes. The hash was only passed as
string to allow `bpftrace` scripts to print the hash. It was
(incorrectly) assumed that `bpftrace` cannot hex-format and print the
block hash given only the hash as bytes.
The block hash can be printed in `bpftrace` by calling
`printf("%02x")` for each byte of the hash in an `unroll () {...}`.
By starting from the last byte of the hash, it can be printed in
big-endian (the block-explorer format).
```C
$p = $hash + 31;
unroll(32) {
$b = *(uint8*)$p;
printf("%02x", $b);
$p -= 1;
}
```
See also: #22902 (comment)
This is a breaking change to the block_connected tracepoint API, however
this tracepoint has not yet been included in a release.
ACKs for top commit:
laanwj:
Concept and code review ACK 53c9fa9e62
jb55:
ACK 53c9fa9e62
Tree-SHA512: f1b9e4e0ee45aae892e8bf38e04b5ee5fbc643d6e7e27d011b829ed8701dacf966a99b7c877c46cca8666b894a375633e62582c552c8203614c6f2b9c4087585
9de0d94508 doc: add disclaimer highlighting shortcomings of the basic multisig example (Michael Dietz)
f9479e4626 test, doc: basic M-of-N multisig minor cleanup and clarifications (Michael Dietz)
e05cd0546a doc: add another signing flow for multisig with descriptor wallets and PSBTs (Michael Dietz)
17dd657300 doc: M-of-N multisig using descriptor wallets and PSBTs, as well as a signing flow (Michael Dietz)
1f20501efc test: add functional test for multisig flow with descriptor wallets and PSBTs (Michael Dietz)
Pull request description:
Aims to resolve issue https://github.com/bitcoin/bitcoin/issues/21278. I try to follow the steps laanwj outlined there exactly, with the exception of using `combinepsbt` instead of `joinpsbts`. I wrote a functional test to make sure it works as expected before doing the docs, and figured it would also be a good source of documentation. So I kept the test as simple as possible and didn't go crazy with edge-cases and various checks. I do have a lot more test-cases I've written that I will follow up with (either in a separate PR or another commit - lmk if you have a preference), but I want to do it in a way that doesn't bloat this test so it remains useful as a quickstart (unless that's a bad idea)?
ACKs for top commit:
S3RK:
Code review ACK 9de0d94. Rspigler's argument convinced me that we should leave the workflow with two wallets. I assume using multisig with external signers is a popular use-case and it's important to keep compatibility.
laanwj:
Code and documentation review ACK 9de0d94508
Tree-SHA512: 6c76e787c21f09d8be5eaa11f3ca3eaa4868497824050562bdfb2095c73b90f5e8987a8775119891d6bfde586e3f31ad1b13e4b67b0802e1d23ef050227a1211
The tracepoint `validation:block_connected` was introduced in #22006.
The first argument was the hash of the connected block as a pointer
to a C-like String. The last argument passed the hash of the
connected block as a pointer to 32 bytes. The hash was only passed as
string to allow `bpftrace` scripts to print the hash. It was
(incorrectly) assumed that `bpftrace` cannot hex-format and print the
block hash given only the hash as bytes.
The block hash can be printed in `bpftrace` by calling
`printf("%02x")` for each byte of the hash in an `unroll () {...}`.
By starting from the last byte of the hash, it can be printed in
big-endian (the block-explorer format).
```C
$p = $hash + 31;
unroll(32) {
$b = *(uint8*)$p;
printf("%02x", $b);
$p -= 1;
}
```
See also: https://github.com/bitcoin/bitcoin/pull/22902#discussion_r705176691
This is a breaking change to the block_connected tracepoint API, however
this tracepoint has not yet been included in a release.
fac62e6ff5 test: Delete generate* calls from TestNode (MarcoFalke)
fac7f6102f test: Use generate* node RPC, not wallet RPC (MarcoFalke)
faac1cda6e test: Use generate* from TestFramework, not TestNode (MarcoFalke)
Pull request description:
Deleting the methods is needed for #22567 to pave the way to make it easier to implicitly call the `sync_all` member function.
Without the methods being deleted, nothing prevents developers from adding calls to it. As history showed, developers *will* add calls to it. For example, see commit eb02dbba3c from today or the first commit in this pull request.
ACKs for top commit:
stratospher:
Tested ACK fac62e6.
brunoerg:
tACK fac62e6ff5
promag:
Code review ACK fac62e6ff5.
Tree-SHA512: 6d4dea8f95ead954acfef2e6a5d98897ce0c2d02265c5b137bb149d0265543bd51d7e8403e1945b9af75df5524ca50064fe1d2a432b25c8abc71bbb28ed6ed53
bda620aecd test: check abandoned tx in listsinceblock (brunoerg)
Pull request description:
This PR tests if the abandoned transaction is correct in listsinceblock return (wallet_abandonconflict.py).
ACKs for top commit:
jonatack:
ACK bda620aecd
theStack:
ACK bda620aecd
stratospher:
Tested ACK bda620a. This PR verifies whether the transaction txAB1 has been abandoned in listsinceblock and is a nice addition to the test!
Tree-SHA512: e4dce344cf621de7a8b5bd8660d252419772a293080fc881f6f448b6df85c6b1c8f0df619e855a40b6393f53c836f0d7fadbd3916c20cccd3a95830b8b502991
Rather than subsequently calling `gettransaction` and
`decoderawtransaction` to get the decoded information for a specific
tx-id, we can simply use the verbose version of `gettransaction`, which
returns this in a 'decoded' key. I.e.
node.decoderawtransaction(node.gettransaction(txid)['hex'])
can be replaced by:
node.gettransaction(txid=txid, verbose=True)['decoded']
a46f71bb70 lint: enable mypy checking for missing imports (josibake)
22e652662b lint mypy 0.910 (fanquake)
6ae9c2ef23 lint: install pyzmq (22.3.0) into linter environment (josibake)
b93e2299da doc: remove pointlessly duplicated linter version / install info (fanquake)
Pull request description:
This is #22844 with issues addressed, and two additional commits. One to move to the latest mypy version in the CI, and another to remove what is just pointless duplication from the test README. There's no need to relist package versions and install instructions (meaning 2x the work when changing anything), when you can just link to `04_install.sh` which has the versions used and pip invocations to install them.
ACKs for top commit:
practicalswift:
cr ACK a46f71bb70
Tree-SHA512: 2900dea3901d03a846dc1ea912f217d152e803845516c7d941745ec1291d145590cd4bf2ddc497f7cf628119ba9905d7b1531836062aa85b384e39cf436f62c6
The COIN constant is critical in understanding Bitcoin's supply, but what it represents isn't clear from the name of the constant. Adding a comment clarifies the meaning of the constant for future readers.
For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
still some wallet tests that test legacy wallet things. Those remain
unchanged.
The subtract fee from outputs assumes that the leftover input amount
will be dropped to fees. However this only happens if that amount is
less than the cost of change. In the event that it is higher than the
cost of change, the leftover amount will actually become a change
output. To avoid this scenario, force a change type which has a high
cost of change.
To avoid issues with test data leaking across tests cases, the global
vCoins and testWallet are removed from coinselector_tests and all of the
relevant functions reworked to not need them.
146831f80a ci: Reduce Windows memory for faster scheduling (MarcoFalke)
Pull request description:
A rebased fac3ae2333 from #23043.
The worst scenario (all caches are invalidated) tested in #23217.
ACKs for top commit:
MarcoFalke:
cr ACK 146831f80a
Tree-SHA512: 8d5101a7a47139f5358601def3932d199da500ba255740c09a170378a3e6fd9b0ff4ce5a7c0f5f1eb4f26f905f55b47077759b265b67acf9b7b01eec89b7e5da
67bb6b5c43 ci, refactor: Disable binaries for Android task explicitly (Hennadii Stepanov)
Pull request description:
This PR defines a set of binaries that are compiled in the Android APK task explicitly via `configure` options, that allows to avoid relying on the `test_bitcoin_qt` target from the `src/qt/Makefile`.
It is ported from https://github.com/bitcoin-core/gui-qml/pull/58.
No behavior change.
ACKs for top commit:
MarcoFalke:
cr ACK 67bb6b5c43
Tree-SHA512: 9aee1083489a69a40e6779291aba423816f59a1fe6a2156be4edafd0c1c5dd14b99215ca4ff0ec32562e0f43f6ed38e4f8ee562020649be589d258156cea86ab
