ParsePrevouts uses GetScriptForWitness on the given witnessScript
to find the corresponding redeemScript. This is incorrect when the
witnessScript is either a P2PK or P2PKH script as it returns the
corresponding P2WPK script instead of turning the witnessScript
into a P2WSH script. Instead this should make the script a
WitnessV0ScriptHash destination and get the script for that.
Test cases are also added.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it will treat the last block processed as the
current tip.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it may set a different lock time.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change affects behavior in a few small ways.
- If there's no max_height specified, percentage progress is measured ending at
wallet last processed block instead of node tip
- More consistent error reporting: Early check to see if start_block is on the
active chain is removed, so start_block is always read and the triggers an
error if it's unavailable
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
https://github.com/bitcoin/bitcoin/issues/14338#issuecomment-426706574
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it may use a more accurate rescan
time.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it will use more accurate backup and
rescan timestamps.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case the "Block not found in chain" error
will be stricter and not allow importing data from a blocks between the wallet
last processed tip and the current node tip.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
It also helps ensure the GUI display stays up to date in the case where the
node chain height runs ahead of wallet last block processed height.
FoundBlock class allows interfaces::Chain::findBlock to return more block
information without having lots of optional output parameters. FoundBlock class
is also used by other chain methods in upcoming commits.
There is mostly no change in behavior. Only exception is
CWallet::RescanFromTime now throwing NonFatalCheckError instead of
std::logic_error.
41b0baf43c gui: Handle WalletModel::unload asynchronous (João Barbosa)
ab31b9d6fe Fix wallet unload race condition (Russell Yanofsky)
Pull request description:
This PR consists in two fixes. The first fixes a concurrency issues with `boost::signals2`. The second fixes a wallet model destruction while it's being used.
From boost signal documentation at https://www.boost.org/doc/libs/1_72_0/doc/html/signals2/thread-safety.html:
> When a signal is invoked by calling signal::operator(), the invocation first acquires a lock on the signal's mutex. Then it obtains a handle to the signal's slot list and combiner. Next it releases the signal's mutex, before invoking the combiner to iterate through the slot list.
This means that `UnregisterValidationInterface` doesn't prevent more calls to that interface. The fix consists in capturing the `shared_ptr<CValidationInterface>` in each internal slot.
The GUI bug is fixed by using a `Qt::QueuedConnection` in the `WalletModel::unload` connection.
ACKs for top commit:
ryanofsky:
Code review ACK 41b0baf43c. Only change is moving assert as suggested
hebasto:
ACK 41b0baf43c, tested on Linux Mint 19.3.
Tree-SHA512: 4f712d8de65bc1214411831250de5dc0a9fd505fb84da5baf9f2cc4d551bc3abffc061616f00afe43dba7525af2cd96c9b54aeead9383145e3b8801f25d85f50
0933a37078 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa)
Pull request description:
Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not.
The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135.
ACKs for top commit:
hebasto:
ACK 0933a37078, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
jonasschnelli:
utACK 0933a37078
instagibbs:
ACK 0933a37078
ryanofsky:
Code review ACK 0933a37078, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out.
jonatack:
ACK 0933a37078 based primarily on code review, despite a lot of manual testing with a large 177MB wallet.
Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1
e980214bc4 serialization: prevent int overflow for big Coin::nHeight (pierrenn)
Pull request description:
This is an attempt to fix fuzzer issues 1,2,8 reported by practicalswift here : https://github.com/bitcoin/bitcoin/issues/18046
The fuzzer harness doesn't prevent deserialization of unrealistic high values for `Coin::nHeight`. In the [provided examples](https://github.com/bitcoin/bitcoin/issues/18046), we have :
- `blockundo_deserialize` : the varint `0x8DD88DD700` is deserialized as `3944983552` in `Coin::nHeight` (`TxInOutFormatter::Unser`)
- `coins_deserialize` : the varint `0x8DD5D5EC40` is deserialized as `3939874496` similarly
- `txundo_deserialize`: the varint `0x8DCD828F01` is deserialized as `3921725441` in `Coin::nHeight` (`Coin::Unserialize`)
Since `Coin::nHeight` is 31 bit long, multiplying a large value by 2 triggers the fuzzer.
AFAIK those values are unrealistic (~70k years for the smallest..). I've looked a bit a reducing the range of values the fuzzer can deserialize, but this seems to be too much code change for not much.
Hence this PR chooses to static cast `nHeight` when re-serializing; it seems to be the less intrusive/safest way to prevent the fuzzer output.
Another more "upstream" approach would be to limit `Coin::nHeight` values to something more realistic, e.g. `0xFFFFFFF` (~5k years) :
de3a30bab2/src/undo.h (L39) and de3a30bab2/src/coins.h (L71)
Thanks !
NB: i was also not sure about the component/area to prefix the PR/commit with.. ?
ACKs for top commit:
practicalswift:
ACK e980214bc4 -- patch looks correct
promag:
ACK e980214bc4.
sipa:
utACK e980214bc4
MarcoFalke:
re-ACK e980214bc4🎑
ryanofsky:
Code review ACK e980214bc4. Just removed ternary ? 1 : 0 and replaced / 2 with >> 1 since last review
Tree-SHA512: 905fc9e5e52a6857abee4a1c863751767835965804bb8c39474f27a120f65399ff4ba7a49ef1da0ba565379f8c12095bd384b6c492cf06776f01b2db68d522b8
11a520f679 tests: Add fuzzing harness for functions/classes in random.h (practicalswift)
64d277bbbc tests: Add fuzzing harness for LimitedString (serialize.h) (practicalswift)
f205cf7fef tests: Add fuzzing harness for functions/classes in span.h (practicalswift)
9718f38f54 tests: Add fuzzing harness for functions/classes in merkleblock.h (practicalswift)
a16ea051f9 tests: Add fuzzing harness for functions/classes in flatfile.h (practicalswift)
Pull request description:
* Add fuzzing harness for functions/classes in `flatfile.h`
* Add fuzzing harness for functions/classes in `merkleblock.h`
* Add fuzzing harness for functions/classes in `span.h`
* Add fuzzing harness for `LimitedString` (`serialize.h`)
* Add fuzzing harness for functions/classes in `random.h`
Top commit has no ACKs.
Tree-SHA512: 6f7e0f946f1062d51216990cde9672b4e896335152548ace3d8711e4969c3e3c8566d01d915b72adcda5c1caa9c2e34da6b7473b55a229f5b77239d3b0ba4b67
621e86ee8d Update -blocksonly documentation (glowang)
Pull request description:
When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0.
This behavior is not captured by the current documentation, which
claims that -blocksonly does not impact any wallet transactions at
all.
Fixes#17294
ACKs for top commit:
MarcoFalke:
ACK 621e86ee8d
Tree-SHA512: f47bfb40a196c23e62505e1d4f79094011ac7c21fc9b920fad60cdadb5c4f48e993be1f015e26e568ce329967c24848fd7b665a6cffd3881f4cfcd2fd0081ed8
When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0 if it has not been set already.This behavior
is not captured by the current documentation, which claims that -blocksonly
does not impact any wallet transactions.
Update the max number of outgoing peers from 8 to 10, due to the
addition of two -blocksonly peers.
6e0d82c55b rpc: remove unused getbalances() code (Jon Atack)
Pull request description:
This line from 999931cf8f appears to be extraneous and replaced 2 lines after by `UniValue balances{UniValue::VOBJ};`.
ACKs for top commit:
Empact:
ACK 6e0d82c55b
hebasto:
ACK 6e0d82c55b, the `obj` local variable is not used until the end of the scope.
Tree-SHA512: a220ca9cda091e78144d9b7fbe4bf90e8338d6e8c8dc7bea27a8e62f3a8ac1d983ad12a48a0a3366b2d8b9586878dfc69c1ec34bf846b34c91e42cda48a59850
c34164896c Bugfix: RPC: Remove final comma for last entry of fixed-size Arrays and Objects in RPCResult (Luke Dashjr)
Pull request description:
JSON doesn't allow a trailing comma in arrays
Top commit has no ACKs.
Tree-SHA512: 761502a05f447afc09c120f13bf23abd2aee83a7f5e5dadaf54c7e1c0c1280d83ee041ca6ca45998fb561e41b32d01067ec52a187c3bcc9d53303ea813bc212c
faaf1cb5b9 util: Replace i64tostr with ToString (MarcoFalke)
fac96fff62 util: Remove unused itostr (MarcoFalke)
Pull request description:
Currently unused, but if someone really needed to use a helper with this functionality in the future, they could use `ToString`.
ACKs for top commit:
laanwj:
ACK faaf1cb5b9
promag:
Code review ACK faaf1cb5b9.
Tree-SHA512: 42180c03f51d677f7b69da23c7868bdd88944335fad0752fcc307f2c3e3c69f1cc1b316ac0875bcefb9a69c5d55200d7cf66843ea4c0f0f26baf7a054b96c1bb
ff9c671b11 refactor: Work around GCC 9 `-Wredundant-move` warning (Russell Yanofsky)
b837b334db net: Fail instead of truncate command name in CMessageHeader (Wladimir J. van der Laan)
Pull request description:
Fixes all 3 from #16992 (see commits)
- net: Fail instead of truncate command name in CMessageHeader
- refactor: Use std::move workaround for unique_ptr upcast only when necessary
ACKs for top commit:
practicalswift:
ACK ff9c671b11 -- patch looks correct
sipa:
utACK ff9c671b11
ryanofsky:
Code review ACK ff9c671b11. Looks good and seems to pass travis, modulo a timeout on one build
hebasto:
ACK ff9c671b11, tested on Fedora 31:
Tree-SHA512: 52d8c13aaf0d56f9bc546a98d7f853eae21f7e325b202fdeb2286b19a9a0ee308634c644b039f60ad8043421e382381cbf1bce58d9f807547f928621c7d245d0
Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.
To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.
ef35604c9c rpc: fix broken RPCExamples for waitforblock(height) (Sebastian Falbesoner)
Pull request description:
This PR fixes several broken RPCExamples from the "blockchain" category:
- `HelpExampleCli` for `waitforblock` (disturbing comma between arguments)
- `HelpExampleCli` for `waitforblockheight` (disturbing comma between arguments)
- `HelpExampleRpc` for `waitforblockheight` (disturbing quotation marks around integer argument)
Note that the CLI example for `waitforblockheight` would also work with the first argument in quotation marks (in contrast to the RPC example), but I removed them as well as they are not needed.
Outputs for the non-working examples in the master branch:
```
$ ./bitcoin-cli waitforblock "0000000000079f8ef3d2c688c244eb7a4570b24c9ed7b4a8c619eb02596f8862", 1000
error code: -8
error message:
blockhash must be of length 64 (not 65, for '0000000000079f8ef3d2c688c244eb7a4570b24c9ed7b4a8c619eb02596f8862,')
```
```
$ ./bitcoin-cli waitforblockheight "100", 1000
error: Error parsing JSON:100,
```
```
$ curl --user __cookie__ --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "waitforblockheight", "params": ["100", 1000]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
Enter host password for user '__cookie__':
{"result":null,"error":{"code":-1,"message":"JSON value is not an integer as expected"},"id":"curltest"}
```
Outputs for the fixed examples in the PR branch:
```
$ ./bitcoin-cli waitforblock "0000000000079f8ef3d2c688c244eb7a4570b24c9ed7b4a8c619eb02596f8862" 1000
{
"hash": "0000000000000000000910ae4d56120e0ddd55c0552e80ed12dba147abc68080",
"height": 622416
}
```
```
$ ./bitcoin-cli waitforblockheight 100 1000
{
"hash": "0000000000000000000910ae4d56120e0ddd55c0552e80ed12dba147abc68080",
"height": 622416
}
```
```
$ curl --user __cookie__ --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "waitforblockheight", "params": [100, 1000]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
Enter host password for user '__cookie__':
{"result":{"hash":"0000000000000000000910ae4d56120e0ddd55c0552e80ed12dba147abc68080","height":622416},"error":null,"id":"curltest"}
```
ACKs for top commit:
fanquake:
ACK ef35604c9c
Tree-SHA512: b98c6681d1aa24b3ee3ef4ef450cb630082a9f8695af18f3b6d418e5b0b1e472b787ccf6397cd719b4d5fe0082ea5f1d0ca553c1cc56066ee2d288be34c601e3
9ab14e4d21 Limit decimal range of numbers ParseScript accepts (pierrenn)
Pull request description:
Following up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18413#issuecomment-602966490, prevent the output of `atoi64` in the `core_read.cpp:ParseScript` helper to send to `CScriptNum::serialize` values wider than 32-bit.
Since the `ParseScript` helper is only used by the tool defined in `bitcoin-tx.cpp`, this only prevents users to provide too much unrealistic values.
ACKs for top commit:
laanwj:
ACK 9ab14e4d21
Tree-SHA512: ee228269d19d04e8fee0aa7c0ae2bb0a2b437b8e574356e8d9b2279318242057d51fcf39a842aa3afe27408d0f2d5276df245d07a3f4828644a366f80587b666
