Age | Commit message (Collapse) | Author |
|
|
|
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
edbed31066e3674ba52b8c093ab235625527f383 chainparams: add signet assumeutxo param at height 160_000 (Sjors Provoost)
b8cafe38713cbf10d15459042f7f911bcc1b1e4e chainparams: add testnet assumeutxo param at height 2_500_000 (Sjors Provoost)
99839bbfa7110c7abf22e587ae2f72c9c57d3c85 doc: add note about confusing HaveTxsDownloaded name (James O'Beirne)
7ee46a755f1d57ce9d51975d3b54dc9ac3d08d52 contrib: add script to demo/test assumeutxo (James O'Beirne)
42cae39356fd20d521aaf99aff1ed85856f3c9f3 test: add feature_assumeutxo functional test (James O'Beirne)
0f64bac6030334d798ae205cd7af4bf248feddd9 rpc: add getchainstates (James O'Beirne)
bb0585779472962f40d9cdd9c6532132850d371c refuse to activate a UTXO snapshot if mempool not empty (James O'Beirne)
ce585a9a158476b0ad3296477b922e79f308e795 rpc: add loadtxoutset (James O'Beirne)
62ac519e718eb7a31dca1102a96ba219fbc7f95d validation: do not activate snapshot if behind active chain (James O'Beirne)
9511fb3616b7bbe1d0d2f54a45ea0a650ba0367b validation: assumeutxo: swap m_mempool on snapshot activation (James O'Beirne)
7fcd21544a333ffdf1910b65c573579860be6a36 blockstorage: segment normal/assumedvalid blockfiles (James O'Beirne)
4c3b8ca35c2e4a441264749bb312df2bd054b5b8 validation: populate nChainTx value for assumedvalid chainstates (James O'Beirne)
49ef778158c43859946a592e11ec34fe1b93a5b6 test: adjust chainstate tests to use recognized snapshot base (James O'Beirne)
1019c399825b0d512c1fd751c376d46fed4992b9 validation: pruning for multiple chainstates (James O'Beirne)
373cf91531b84bfdd06fdf8abf4dca228029ce6b validation: indexing changes for assumeutxo (James O'Beirne)
1fffdd76a1bca908f55d73b64983655b14cf7432 net_processing: validationinterface: ignore some events for bg chain (James O'Beirne)
fbe0a7d7ca680358237b6c2369b3fd2b43221113 wallet: validationinterface: only handle active chain notifications (James O'Beirne)
f073917a9e7ba423643dcae0339776470b628f65 validationinterface: only send zmq notifications for active (James O'Beirne)
4d8f4dcb450d31e4847804e62bf91545b949fa14 validation: pass ChainstateRole for validationinterface calls (James O'Beirne)
1e59acdf17309f567c370885f0cf02605e2baa58 validation: only call UpdatedBlockTip for active chainstate (James O'Beirne)
c6af23c5179cc383f8e6c275373af8d11e6a989f validation: add ChainstateRole (James O'Beirne)
9f2318c76cc6986d48e13831cf5bd8dab194fdf4 validation: MaybeRebalanceCaches when chain leaves IBD (James O'Beirne)
434495a8c1496ca23fe35b84499f3daf668d76b8 chainparams: add blockhash to AssumeutxoData (James O'Beirne)
c711ca186f8d8a28810be0beedcb615ddcf93163 assumeutxo: remove snapshot during -reindex{-chainstate} (James O'Beirne)
c93ef43e4fd4fbc1263cdc9e98ae5856830fe89e bugfix: correct is_snapshot_cs in VerifyDB (James O'Beirne)
b73d3bbd23220857bf17cbb6401275bf58013b72 net_processing: Request assumeutxo background chain blocks (Suhas Daftuar)
Pull request description:
- Background and FAQ: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
- Prior progress/project: https://github.com/bitcoin/bitcoin/projects/11
- Replaces https://github.com/bitcoin/bitcoin/pull/15606, which was closed due to Github slowness. Original description and commentary can be found there.
---
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (`loadtxoutset`) and adds `assumeutxo` parameters to chainparams. It contains all the remaining changes necessary to both use an assumedvalid snapshot chainstate and do a full validation sync in the background.
This may look like a lot to review, but note that
- ~200 lines are a (non-essential) demo shell script
- Many lines are functional test, documentation, and relatively dilute RPC code.
So it shouldn't be as burdensome to review as the linecount might suggest.
- **P2P**: minor changes are made to `init.cpp` and `net_processing.cpp` to make simultaneous IBD across multiple chainstates work.
- **Pruning**: implement correct pruning behavior when using a background chainstate
- **Blockfile separation**: to prevent "fragmentation" in blockfile storage, have background chainstates use separate blockfiles from active snapshot chainstates to avoid interleaving heights and impairing pruning.
- **Indexing**: some `CValidationInterface` events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.
- Have `-reindex` properly wipe snapshot chainstates.
- **RPC**: introduce RPC commands `loadtxoutset` and (hidden) `getchainstates`.
- **Release docs & first assumeutxo commitment**: add notes and a particular assumeutxo hash value for first AU-enabled release.
- This will complete the project and allow use of UTXO snapshots for faster node bootstrap.
The next phase, if it were to be pursued, would be coming up with a way to distribute the UTXO snapshots over the P2P network.
---
### UTXO snapshots
Create your own with `./contrib/devtools/utxo_snapshot.sh`, e.g.
```shell
./contrib/devtools/utxo_snapshot.sh 788000 utxo.dat ./src/bitcoin-cli -datadir=$(pwd)/testdata`)
```
or use the pre-generated ones listed below.
- Testnet: **2'500'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:511e09f4bf853aefab00de5c070b1e031f0ecbe9&dn=utxo-testnet-2500000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `79db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388`
- Signet: **160'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8c`
- Mainnet: **800'000** (Sjors):
- Note: this needs the following commit cherry-picked in: https://github.com/Sjors/bitcoin/commit/24deb2022b822f22fba9fcbee201e37a83225eb2
- torrent: `magnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
### Testing
#### For fun (~5min)
If you want to do a quick test, you can run `./contrib/devtools/test_utxo_snapshots.sh` and follow the instructions. This is mostly obviated by the functional tests, though.
#### For real (longer)
If you'd like to experience a real usage of assumeutxo, you can do that too.
I've cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with `./contrib/devtools/utxo_snapshot.sh` if you want). Download that, and then create a datadir for testing:
```sh
$ cd ~/src/bitcoin # or whatever
# get the snapshot
$ curl http://img.jameso.be/utxo-788000.dat > utxo-788000.dat
# you'll want to do this if you like copy/pasting
$ export AU_DATADIR=/home/${USER}/au-test # or wherever
$ mkdir ${AU_DATADIR}
$ vim ${AU_DATADIR}/bitcoin.conf
dbcache=8000 # or, you know, something high
blockfilterindex=1
coinstatsindex=1
prune=3000
logthreadnames=1
```
Obtain this branch, build it, and then start bitcoind:
```sh
$ git remote add jamesob https://github.com/jamesob/bitcoin
$ git fetch jamesob assumeutxo
$ git checkout jamesob/assumeutxo
$ ./configure $conf_args && make # (whatever you like to do here)
# start 'er up and watch the logs
$ ./src/bitcoind -datadir=${AU_DATADIR}
```
Then, in some other window, load the snapshot
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
```
You'll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you'll see the occasional `[background validation]` log message go by.
In yet another window, you can check out chainstate status with
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
```
as well as usual favorites like `getblockchaininfo`.
ACKs for top commit:
achow101:
ACK edbed31066e3674ba52b8c093ab235625527f383
Tree-SHA512: 6086fb9a38dc7df85fedc76b30084dd8154617a2a91e89a84fb41326d34ef8e7d7ea593107afba01369093bf8cc91770621d98f0ea42a5b3b99db868d2f14dc2
|
|
380130d9d70f8f8d395949a51f43806f6e600efa test: add coverage to feature_addrman.py (kevkevin)
Pull request description:
I added two new tests that will cover the nNew and nTried tests which add coverage to the if block by checking values larger than our range since we only check for negative values now
adding coverage to these lines
https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L273
https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L280
our test seem to only cover the `nTried < 0` and `nNew < 0` scenarios
ACKs for top commit:
ismaelsadeeq:
ACK 380130d9d70f8f8d395949a51f43806f6e600efa, code looks good to me 🍃 .
0xB10C:
Re-ACK 380130d9d70f8f8d395949a51f43806f6e600efa
Tree-SHA512: a063bd9ca4d2d536a27c8c22a28fb13759a96f19cd8ba6cb8879cf7f65046d4ff6e8f70df17feaffd0d0d08ef914cb18a11258d313a4841c811a7e7ae4df6d5b
|
|
and conflicting heights in MarkConflicted
782701ce7d31919dba2241ee43b582d8ae5a2541 test: Test loading wallets with conflicts without a chain (Andrew Chow)
4660fc82a1f5cf6eb6404d5268beef5919581661 wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow)
Pull request description:
`MarkConflicted` assumes that `m_last_block_processed_height` is always valid. However it may not be valid when a chain is not attached, as happens in the wallet tool and during migration. In such situations, when the conflicting height is also negative (which occurs on loading when no chain is available), the calculation of the number of conflict confirms results in a non-negative value which passes the existing check for valid values. This will subsequently hit an assertion in `GetTxDepthInMainChain`.
Furthermore, `MarkConflicted` is also only called on loading a transaction whose parent has a stored state of `TxStateConflicted` and was loaded before the child transaction. This depends on the loading order, which for both sqlite and bdb depends on the txids.
We can avoid this by explicitly checking that both `m_last_block_processed_height` and `conflicting_height` are non-negative. Both `tool_wallet.py` and `wallet_migration.py` are updated to create wallets with a state that triggers the assertion.
Fixes #28510
ACKs for top commit:
ryanofsky:
Code review ACK 782701ce7d31919dba2241ee43b582d8ae5a2541. Nice catch, and clever test (grinding the txid)
furszy:
ACK 782701ce
Tree-SHA512: 1344e0279ec5413a43a2819d101fb571fbf4821de2d13958a0fdffc99f57082ef3243ec454c8343f97dc02ed1fce8c8b0fd89388420ab2e55618af42ad5630a9
|
|
reasons, add test coverage
2ab7952bda8d15e91b03f8307839030cbb55614e test: add bip157 coverage for (start height > stop height) disconnect (Sebastian Falbesoner)
63e90e1d3f5ed08f9871f07667d389ec66aa621c test: check for specific disconnect reasons in p2p_blockfilters.py (Sebastian Falbesoner)
Pull request description:
This PR checks for specific disconnect reasons using `assert_debug_log` in the functional test `p2p_blockfilters.py`. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message.
Also, based on that, missing coverage for the (start height > stop height) disconnect case is added:
https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/src/net_processing.cpp#L3050-L3056
ACKs for top commit:
MarcoFalke:
lgtm ACK 2ab7952bda8d15e91b03f8307839030cbb55614e
furszy:
Looks good, code ACK 2ab7952b
Tree-SHA512: 0581cb569d5935aaa004a95a6f16eeafe628b9d816ebb89232f2832e377049df878a1e74c369fb46931b94e1a3a5e3f4aaa21a007c0a488f4ad2cda0919c605d
|
|
cap-add LINUX_IMMUTABLE
fa40b3ee22e78f58d7426dbc4343472ba40081e3 test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE (MarcoFalke)
Pull request description:
This turns a test failure on Linux when running the test as `root`, but without the `LINUX_IMMUTABLE` capability, into an early return, with a suggestion to turn on `LINUX_IMMUTABLE` next time (if possible).
ACKs for top commit:
pinheadmz:
utACK fa40b3ee22e78f58d7426dbc4343472ba40081e3
jonatack:
ACK fa40b3ee22e78f58d7426dbc4343472ba40081e3
Tree-SHA512: d986ff8aeae5f8267c21a23d5be16f7c5a4d4d3be045a6999d8b39c7b8672cfe915dedde762cc9965cdc4970940bffc4b0d1412833d8036d4425450eb6181f67
|
|
I added two new tests that will cover the nNew and nTried tests which
add coverage to the if block by checking values larger than our range
since we only check for negative values now
Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>
|
|
Most ideas for test improvements (TODOs) provided by Russ Yanofsky.
|
|
implementation
96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983 test: add unit test coverage for Python ECDSA implementation (Sebastian Falbesoner)
Pull request description:
This PR adds missing unit test coverage for the Python ECDSA implementation, which should be useful for detecting potential problems early whenever changes in the test framework's Python implementation of secp256k1 are made (e.g. #26222). Note that right now we don't call `ECPubKey.verify_ecdsa` anywhere in our tests, so we wouldn't notice if it is broken at some point.
To keep it simple, the already existing unit test for Schnorr signatures is extended to also check ECDSA signatures. For that purpose, the dictionary storing private-key/public-key entries use their legacy types `ECKey/ECPubKey` instead of bare byte-arrays, and for Schnorr signing/verification the necessary conversions (ECKey -> bare private key, ECPubKey -> x-only pubkey) is done later when needed. To avoid code duplication, a helper function `random_bitflip` for damaging signatures is introduced.
The unit test can be run by either calling it for this single module:
`$ python3 -m unittest ./test/functional/test_framework/key.py`
or simply running `$ ./test/functional/test_runner.py` which calls all test framework module's unit tests at the start (see TEST_FRAMEWORK_MODULES list).
ACKs for top commit:
achow101:
ACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983
sipa:
utACK 96b3f2dbe4395ca55cfdd58c8f9f9bd7ca163983
stratospher:
tested ACK 96b3f2d.
Tree-SHA512: b993f25b843fa047376addda4ce4b0f15750ffba926528b5cca4c5f99b9af456206f4e8af885d25a017dddddf382ddebf38765819b3d16a3f28810d03b010808
|
|
intermittent failure by using manual bumps instead of bumpfee
b5a962564eb15075e4e2a7bc0c235a56fa998ac3 tests: Use manual bumps instead of bumpfee for resendwallettransactions (Andrew Chow)
Pull request description:
Bumpfee will try to increase the entire package to the target feerate, which causes repeated bumpfees to quickly shoot up in fees, causing intermittent failures when the fee is too large. We don't care about this property, just that the child is continuously replaced until we observe it's position in mapWallet is before its parent. Instead of using bumpfee, we can create raw transactions which have only pay (just above) the additional incremental relay fee, thus avoiding this problem.
Fixes #28491
ACKs for top commit:
kevkevinpal:
ACK [b5a9625](https://github.com/bitcoin/bitcoin/pull/28540/commits/b5a962564eb15075e4e2a7bc0c235a56fa998ac3)
mzumsande:
Code review ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3
pablomartin4btc:
ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3 -> adding the `try_rpc` to avoid (skip) any possible failure around the manual bump fee (if we ever reach it as [explained](https://github.com/bitcoin/bitcoin/pull/28540#issuecomment-1737648048)) makes a lot of sense as the spirit of the test is the tx (child before parent) sort in the `mapWallet` (as also [explained](https://github.com/bitcoin/bitcoin/issues/28491#issuecomment-1736161363)).
MarcoFalke:
lgtm ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3
Tree-SHA512: f184f11c73be0c30753181901f51a3b4b9c4135e0c4681e9f4ca94692c49bac15c91683c85266a2124333c8593e9919bfd9102724616faab299740f2eb98741f
|
|
b3db8c9d5ccfe5c31341169fa7ac044427122921 rpc: bumpfee, improve doc for 'reduce_output' arg (furszy)
Pull request description:
Fixes #28180. Resulted from discussions with S3RK, achow101, and Murch.
The current argument name and description are dangerous as it don't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the inputs
minus outputs remainder. Which, when `bumpfee` adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect from a
'reduce_output' param naming.
ACKs for top commit:
S3RK:
ACK b3db8c9d5ccfe5c31341169fa7ac044427122921
achow101:
ACK b3db8c9d5ccfe5c31341169fa7ac044427122921
murchandamus:
ACK b3db8c9d5ccfe5c31341169fa7ac044427122921
Tree-SHA512: 91f607e2f5849041d7c099afdddae11af8bed5b1ac90c9d22921267f272e21b44e107d6968e037f05f958a61fe29e94e5fb44b224fb3606f197f83ec4ba3b1e7
|
|
Bumpfee will try to increase the entire package to the target feerate,
which causes repeated bumpfees to quickly shoot up in fees, causing
intermittent failures when the fee is too large. We don't care about
this property, just that the child is continuously replaced until we
observe it's position in mapWallet is before its parent. Instead of
using bumpfee, we can create raw transactions which have only pay the
additional incremental relay fee, thus avoiding this problem.
|
|
|
|
Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.
|
|
The current argument name and description are dangerous as it don't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the inputs
minus outputs remainder. Which, when bumpfee adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect from a
'reduce_output' param naming.
Co-authored-by: Murch <murch@murch.one>
|
|
encoded tx if complete
a99e9e655a58b2364a74aec5cafb827a73c6b0c4 doc: add release note (ismaelsadeeq)
2b4edf889a4b555c8c7f6793fa5d820e5513ecac test: check `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
c405207a18fdee75a4dea470bb0d13e59e15ce45 rpc: `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
Pull request description:
Coming from [#28414 comment](https://github.com/bitcoin/bitcoin/pull/28414#pullrequestreview-1618684391) Same thing also for `descriptorprocesspsbt`.
Before this PR `descriptorprocesspsbt` returns a boolean `complete` which indicates that the psbt is final, users then have to call `finalizepsbt` to get the hex encoded network transaction.
In this PR if the psbt is complete the return object also has the hex encoded network transaction ready for broadcast with `sendrawtransaction`.
This save users calling `finalizepsbt` with the descriptor, if it is already complete.
ACKs for top commit:
achow101:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
pinheadmz:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
ishaanam:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
Tree-SHA512: c3f1b1391d4df05216c463127cd593f8703840430a99febb54890bc66fadabf9d9530860605f347ec54c1694019173247a0e7a9eb879d3cbb420f9e8d9839b75
|
|
transaction package context
eb8f58f5e4a249d55338304099e8238896d2316d Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01256b0b2570168496d129a8b2009b35 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3d0bae2ab766a8248219aa3c9e344df Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58ad5a218671a9bc1537299b1d67bb55d Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)
Pull request description:
(Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:
1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
2) pass correct vsize to chain limit evaluations in package context
3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)
This should fix the known issues while not blocking additional refactoring later.
ACKs for top commit:
achow101:
ACK eb8f58f5e4a249d55338304099e8238896d2316d
ariard:
Re-Code ACK eb8f58f5e
glozow:
reACK eb8f58f5e4a249d55338304099e8238896d2316d
Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39
|
|
d05be124dbc0b24fb69d0c28ba2d6b297d243751 test: added coverage to estimatefee (kevkevin)
Pull request description:
Added a assert for an rpc error when we try to estimate fee for the max conf_target
Line I am adding coverage to https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#LL71C52-L71C52
ACKs for top commit:
MarcoFalke:
lgtm ACK d05be124dbc0b24fb69d0c28ba2d6b297d243751
Tree-SHA512: dfab075989446e33d1a5ff1a308f1ba1b9f80cce3848fbe4231f69212ceef456a3f2b19365a42123e0397c31893fd9f1fd9973cc00cfbb324386e12ed0e6bccc
|
|
for invalid command
f52cb02f700b58bca921a7aa24bfeee04760262b doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg)
effd1efefb53c58f0e43fec4f019a19f97795553 test: `addnode` with an invalid command should throw an error (brunoerg)
56b27b84877376ffc32b3bad09f1047b23de4ba1 rpc, refactor: clean-up `addnode` (brunoerg)
Pull request description:
This PR:
- Adds test coverage for an invalid `command` in `addnode`.
- Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones.
- Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
- Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field.
ACKs for top commit:
achow101:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
jonatack:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
theStack:
re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b
Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e
|
|
signing for tx inputs
83d7cfd5429b0be7755bc48145032956f5f56dae test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs (Sebastian Falbesoner)
Pull request description:
This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature:
1. calculate the `SegwitV0SignatureHash` with the desired sighash type
2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above
3. put the DER-encoded result (plus sighash byte) at the bottom of the witness stack
ACKs for top commit:
achow101:
ACK 83d7cfd5429b0be7755bc48145032956f5f56dae
pinheadmz:
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae
Tree-SHA512: b8e55409ddc9ddb14cfc06daeb4730d7750a4632f175f88dcac6ec4d216e71fd4a7eee325a64d6ebba3b33be50bcd30c2de7400f834c01abb67e52840d9823b6
|
|
|
|
|
|
new/tried table address count
28bac81a346c0b68273fa73af924f7096cb3f41d test: add functional test for getaddrmaninfo (stratospher)
c8eb8dae51039aa1938e7040001a149210e87275 rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher)
Pull request description:
implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.
This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.
```jsx
$ getaddrmaninfo
Result:
{ (json object) json object with network type as keys
"network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns)
"new" : n, (numeric) number of addresses in new table
"tried" : n, (numeric) number of addresses in tried table
"total" : n (numeric) total number of addresses in both new/tried tables from a network
},
...
}
```
### additional context from [original PR](https://github.com/bitcoin/bitcoin/pull/26988)
1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851.
2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808.
ACKs for top commit:
0xB10C:
ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
willcl-ark:
reACK 28bac81a346c0b68273fa73af924f7096cb3f41d
achow101:
ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
brunoerg:
reACK 28bac81a346c0b68273fa73af924f7096cb3f41d
theStack:
Code-review ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
|
|
mutating it in package evaluation
ee589d4466bb0548a6f2215afe8abd0735768dab Add regression test for m_limit mutation (Greg Sanders)
275579d8c133c066212a26423639956e2576e97a Remove MemPoolAccept::m_limits, only have local copies for carveouts (Greg Sanders)
Pull request description:
Without remoing it, if we ever call `PreChecks()` multiple times for any reason during any one `MempoolAccept`, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.
Currently this is only an issue with `submitpackage`, so this is not exposed on mainnet.
ACKs for top commit:
achow101:
ACK ee589d4466bb0548a6f2215afe8abd0735768dab
glozow:
ACK ee589d4466bb0548a6f2215afe8abd0735768dab, nits can be ignored
ariard:
Code Review ACK ee589d446
Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
|
|
scripts
8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372ab39386e689b27d15c4d029be239319 wallet: disallow migration of invalid or not-watched scripts (furszy)
Pull request description:
Fixing #28057.
The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means `IsMineInner()` returning
`IsMineResult::INVALID` for them.
Note:
To verify this, can run the test commit on top of master.
`wallet_migration.py` will crash without the bugfix commit.
ACKs for top commit:
achow101:
ACK 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9
Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
|
|
|
|
instead of just scriptPubKey
ad0c469d98c51931b98b7fd937c6ac3eeaed024e wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow)
07d3bdf4ebc06825ea24ab6f7c87aef6a22238c6 Add PubKeyDestination for P2PK scripts (Andrew Chow)
1a98a51c666e9ae77364115775ec2e0ba984e8e0 Allow CNoDestination to represent a raw script (Andrew Chow)
8dd067088d41f021b357d7db5fa5f0a9f61edddc Make WitnessUnknown members private (Andrew Chow)
Pull request description:
For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address.
In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts.
Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct.
`ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`.
Builds on #28244
ACKs for top commit:
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/28246/commits/ad0c469d98c51931b98b7fd937c6ac3eeaed024e
Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
|
|
|
|
Test that if the processed psbt is complete the hex encoded tx
is returned and remove unneccessary rpc call to finalize the
psbt.
|
|
target feerate
f18f9ef4d31c70e2d71ab90a24511692821418c3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944dab09eff30952233f8dfc0b12c4553d5 Bump unconfirmed parent txs to target feerate (Murch)
3e3e05241128f68cf12f73ee06ff997395643885 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da6650715f3e1153b6104bbdae15fcac90 [node] interface to get bump fees (glozow)
c24851be945b2a633ee44ed3c8a501eee5580b62 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8f7d578cd4a8593f41189efca548064 Remove unused imports (Murch)
d2f90c31ef3b8dee5a3e0804ecc62fa1cfec7cd5 Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0211e54e21a1d4a570e5f15294dca72 Match tx names to index in miniminer overlap test (Murch)
Pull request description:
Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
Fixes #9645
Fixes #9864
Fixes #15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
ismaelsadeeq:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
achow101:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
brunoerg:
crACK f18f9ef4d31c70e2d71ab90a24511692821418c3
t-bast:
ACK https://github.com/bitcoin/bitcoin/pull/26152/commits/f18f9ef4d31c70e2d71ab90a24511692821418c3, I reviewed the latest changes and run e2e tests against eclair, everything looks good :+1:
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
|
|
blockstore
de8f9123afbecc3b4f59fa80af8148bc865d0588 test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b624ce87320ec16412f98ab591a5860c ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f2420244c583e218f51cd0d3a3cac6731003 unit test: add coverage for BlockManager (Matthew Zipkin)
Pull request description:
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782
ACKs for top commit:
jonatack:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
achow101:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588
MarcoFalke:
lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶
Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73
|
|
Co-authored-by: Andrew Chow <github@achow101.com>
|
|
At the end of coin selection reduce the fees by the difference between
the individual bump fee estimates and the collective bump fee estimate.
|
|
When a transaction uses an unconfirmed input, preceding this commit it
would not consider the feerate of the parent transaction. Given a parent
transaction with a lower ancestor feerate, this resulted in the new
transaction's ancestor feerate undershooting the target feerate.
This commit changes how we calculate the effective value of unconfirmed UTXOs.
The effective value of unconfirmed UTXOs is decreased by the fee
necessary to bump its ancestry to the target feerate. This also impacts
the calculation of the waste metric: since the estimate for the current
fee is increased by the bump fees, unconfirmed UTXOs current fees appear less
favorable compared to their unchanged long term fees.
This has one caveat: if multiple UTXOs have overlapping ancestries, each
of their individual estimates will account for bumping all ancestors.
|
|
evaluation
32c1dd1ad65af0ad4d36a56d2ca32a8481237e68 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460c3fd1c7eb8070623666d887eefccff0d6 [refactor] split setup in mempool_limit test (glozow)
d08696120e3647b4c2cd0ae8d6e57dea12418b7c [test framework] add ability to spend only confirmed utxos (glozow)
3ea71feb11c261f002ed918f91f3434fd8a23589 [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b7234cd4cfd7c593ffcf8e2f24573d1ebea5 [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81828ff98820fa49c83ca364063233374c6 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad33929ee846a55a43c55732be0cb8973060 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c11ca0705e1d6147b90da33ce555f9f41c8 [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3dab1c4ee37fd4093b6a0a3b622f53e231d [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1189432b1b6245ba25df572229870567cb [policy] check for duplicate txids in package (glozow)
Pull request description:
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.
Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.
Pointed out by instagibbs in https://github.com/bitcoin/bitcoin/commit/faeed687e5cde5e32750d93818dd1d4add837f24 on top of the v3 PR.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/28251/commits/32c1dd1ad65af0ad4d36a56d2ca32a8481237e68
Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90
|
|
Test for scenario(s) outlined in PR 28251.
Test what happens when a package transaction spends a mempool coin which
is fetched and then disappears mid-package evaluation due to eviction or
replacement.
|
|
We want to be able to re-use fill_mempool so that none of the tests
affect each other.
Change the logs from info to debug because they are otherwise repeated
many times in the test output.
|
|
Useful to ensure that the topologies of packages/transactions are as
expected, preventing bugs caused by having unexpected mempool ancestors.
|
|
Duplicates of normal transactions would be found by looking for
conflicting inputs, but this doesn't catch identical empty transactions.
These wouldn't be valid but exiting early is good and AcceptPackage's
result sanity checks assume non-duplicate transactions.
|
|
walletprocesspsbt if complete
2e249b922762f19d6ae61edaad062f31bc2849f3 doc: add release note for PR #28414 (Matthew Zipkin)
4614332fc4514f63fcbe9e6de507f7bb9b7e87e9 test: remove unnecessary finalizepsbt rpc calls (ismaelsadeeq)
e3d484b603abff69c6ebfca5cfb78cf82743d090 wallet rpc: return final tx hex from walletprocesspsbt if complete (Matthew Zipkin)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/28363#discussion_r1315753887
`walletprocesspsbt` currently returns a base64-encoded PSBT and a boolean indicating if the tx is "complete". If it is complete, the base64 PSBT can be finalized with `finalizepsbt` which returns the hex-encoded transaction suitable for `sendrawtransaction`.
With this patch, `walletprocesspsbt` return object will ALSO include the broadcast-able hex string if the tx is already final. This saves users the extra step of calling `finalizepsbt` assuming they have already inspected and approve the transaction from earlier steps.
ACKs for top commit:
ismaelsadeeq:
re ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
BrandonOdiwuor:
re ACK 2e249b9
Randy808:
Tested ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
achow101:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
ishaanam:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
Tree-SHA512: 229c1103265a9b4248f080935a7ad5607c3be3f9a096a9ab6554093b2cd8aa8b4d1fa55b1b97d3925ba208dbc3ccba4e4d37c40e1491db0d27ba3d9fe98f931e
|
|
P2PK scripts are not PKHash destinations, they should have their own
type.
This also results in no longer showing a p2pkh address for p2pk outputs.
However for backwards compatibility, ListCoinst will still do this
conversion.
|
|
|
|
(27831 follow-ups)
9f55773a370a0d039e727445ccee6b84e05f562a test: refactor: usdt_mempool: store all events (stickies-v)
bc432704505eb165dd86de39ea3434c6fb7a2514 test: refactor: remove unnecessary nonlocal (stickies-v)
326db63a6819813db55ba0d01ab4fe80f7a0d818 test: log sanity check assertion failures (stickies-v)
f5525ad6808df6afc38e5c6e4767ab577e30629c test: store utxocache events (stickies-v)
f1b99ac94fb77340c4d3a5b4bbc3df28009bc773 test: refactor: deduplicate handle_utxocache_* logic (stickies-v)
ad90ba36bd930f00753643cd1fe0af72d1c828c2 test: refactor: rename inbound to is_inbound (stickies-v)
afc0224cdbe73e326addf5fb98a3e95d941f2104 test: refactor: remove unnecessary blocks_checked counter (stickies-v)
Pull request description:
Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to https://github.com/bitcoin/bitcoin/pull/27831#pullrequestreview-1491438045. Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable.
The rationale for each change is in the corresponding commit message.
Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired.
ACKs for top commit:
0xB10C:
ACK 9f55773a370a0d039e727445ccee6b84e05f562a. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable.
Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5
|
|
`p2p_invalid_block`
3eb03803c4111d09c63550a6a6c09afbe3430bf6 test: remove unused variables in `p2p_invalid_block` (brunoerg)
Pull request description:
ACKs for top commit:
stickies-v:
ACK 3eb03803c4111d09c63550a6a6c09afbe3430bf6
Tree-SHA512: eadae1eb323e5562d1ea0aed43ebf0145f0fdbb6cd6d4646bbf1ca89f384820e7b9cb69f0bb04a949e9f8983a879aee8387d6f7ac3d4e4ea027f8892e656fb98
|
|
sync_with_ping
fae0b21e6c7a27f08ea8f8b49198c734f923b5da test: Combine sync_send_with_ping and sync_with_ping (MarcoFalke)
Pull request description:
This reduces bloat, complexity, and makes tests less fragile to intermittent failures, see https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1315648343.
This should not cause any noticeable slowdown, or may even be faster, because active polling will be done at most once.
ACKs for top commit:
glozow:
Concept ACK fae0b21e6c7a27f08ea8f8b49198c734f923b5da
theStack:
ACK fae0b21e6c7a27f08ea8f8b49198c734f923b5da 🏓
Tree-SHA512: 6c543241a7b85458dc7ff6a6203316b80a6227d83d38427e74f53f4c666a882647a8a221e5259071ee7bb5dfd63476fb03c9b558a1ea546734b14728c3c619ba
|
|
|