fa37e798b2660d8e44e31c944a257b55aeef5de2 wallet: Replace confusing getAdjustedTime() with GetTime() (MarcoFalke)
Pull request description:
Setting `nTimeReceived` to the adjusted time has several issues:
* `m_best_block_time` is set to the "unadjusted" time, thus a comparison of the two times is like comparing apples to oranges. In the worst case this opens up an attack vector where remote peers can force a premature re-broadcast of wallet txs.
* The RPC documentation for `"timereceived"` doesn't mention that the network adjusted time is used, possibly confusing users when the time reported by RPC is off by a few seconds compared to their local timestamp.
Fix all issues by replacing the call with `GetTime()`. Also a style fix: Use non-narrowing integer conversion in the RPC method.
ACKs for top commit:
theStack:
Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2
shaavan:
crACK fa37e798b2660d8e44e31c944a257b55aeef5de2
Tree-SHA512: 8d020ba400521246b7aed4b6c41319fc70552e8c69e929a5994500375466a9edac02a0ae64b803dbc6695df22276489561a23bd6e030c44c97d288f7b9b2b3fa
a989f98d240a84b5c798252acaa4a316ac711189 refactor: net: subnet lookup: use single-result LookupHost() (Sebastian Falbesoner)
Pull request description:
plus describe single IP subnet case for more clarity
ACKs for top commit:
jonatack:
utACK a989f98d240a84b5c798252acaa4a316ac711189 the patch rebases cleanly to master, the debug build is green, and it is essentially the same patch as c8991f0251dd2a modulo local variable naming, braced initialization, and a comment
vasild:
ACK a989f98d240a84b5c798252acaa4a316ac711189
Tree-SHA512: 082d3481b1fa5e5f3267b7c4a812954b67b36d1f94c5296fe20110699f053e5042dfa13f728ae20249e9b8d71e930c3b119410125d0faeccdfbdc259223ee3a6
fa165e954579436fe4b636e4222d8ce0c1269786 Replace stoul with ToIntegral in dbwrapper (MarcoFalke)
Pull request description:
The string is created with `%llu`. See: 7fcf53f7b4/src/leveldb/db/db_impl.cc (L1436-L1437)
So it seems odd to silently accept when parsing: whitespace, a sign character, trailing chars, overflow, ....
Fix that by using the stricter ToIntegral.
ACKs for top commit:
laanwj:
Code review ACK fa165e954579436fe4b636e4222d8ce0c1269786
practicalswift:
cr ACK fa165e954579436fe4b636e4222d8ce0c1269786
theStack:
Code-review ACK fa165e954579436fe4b636e4222d8ce0c1269786
Tree-SHA512: b87f01431ca0b971ff84610022da8679d3c33470b88cfc3f4a337e6e176a0455715588aefd40e8e2bbe7459d902dc89d7bfe34e7fd66755f631cc18dc039fa2f
5b44a75493a1a098404d5e21dc384e74eae1892e refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner)
Pull request description:
As pointed out in issue #17130, the serialization/deserialization methods for the classes `CExtKey` and
`CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543750290, https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543794408 and https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543814727).
ACKs for top commit:
practicalswift:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e -- -60 LOC diff looks correct :)
promag:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e.
MarcoFalke:
unsigned ACK 5b44a75493a1a098404d5e21dc384e74eae1892e
fjahr:
ACK 5b44a75
jonatack:
Light ACK 5b44a75493a1a098404d5e21dc384e74eae1892e. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f16af in 2015 to add bip32 pubkey serialization.
Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c
73b96c94cb6c2afdee7f151768a96944ecaf9d9b net: Fix uninitialized read in ProcessMessage(...) (practicalswift)
Pull request description:
Fix an uninitialized read in `ProcessMessage(…, "tx", …)` when receiving a transaction we already have.
The uninitialized value is read and used on [L2526 in the case of `AlreadyHave(inv) == true`](d8a66626d6/src/net_processing.cpp (L2494-L2526)).
Proof of concept being run against a `bitcoind` built with MemorySanitizer (`-fsanitize=memory`):
```
$ ./p2p-uninit-read-in-conditional-poc.py
Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net>
$ bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
SUMMARY: MemorySanitizer: use-of-uninitialized-value
[1]+ Exit 77 bitcoind -regtest
$
```
Proof of concept being run against a `bitcoind` running under Valgrind (`valgrind --exit-on-first-error`):
```
$ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
==27351== Conditional jump or move depends on uninitialised value(s)
[1]+ Exit 1 valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest
$
```
Proof of concept script:
```
#!/usr/bin/env python3
import sys
from test_framework.mininode import NetworkThread
from test_framework.mininode import P2PDataStore
from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx
def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"):
network_thread = NetworkThread()
network_thread.start()
node = P2PDataStore()
node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)()
node.wait_for_verack()
tx = CTransaction()
tx.vin.append(CTxIn())
tx.vout.append(CTxOut())
node.send_message(msg_tx(tx))
node.send_message(msg_tx(tx))
node.peer_disconnect()
network_thread.close()
if __name__ == "__main__":
if len(sys.argv) != 4:
print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0]))
sys.exit(0)
send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3])
```
Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction.
This bug was introduced in #15921 ("validation: Tidy up ValidationState interface") which was merged in to `master` 28 days ago.
Luckily this bug was caught before being part of any Bitcoin Core release :)
ACKs for top commit:
jnewbery:
utACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b
laanwj:
ACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b, thanks for discovering and reporting this before it ended up in a release.
Tree-SHA512: 7ce6b8f260bcdd9b2ec4ff4b941a891bbef578acf4456df33b7a8d42b248237ec4949e65e2445b24851d1639b10681c701ad500b1c0b776ff050ef8c3812c795
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
9075d13153ce06cd59a45644831ecc43126e1e82 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec0f06fb4fce1c4f93500752f05dede8 [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34ff2d538d8f5315efd9908bf24d0fdc [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714323c1694c834fdca74f064a1a33529 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
cd68594dcdadc195bd2ea9394fa04edfdbdf1149 Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
laanwj:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
promag:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
57e2edea0bfea664e3f12dad2508139eb7f461bc Send amount shows minimum amount placeholder (JeremyCrookshank)
Pull request description:
Noticed that there wasn't a default value for the send amount. However if you put a value in or click the up and down arrows you're unable to get it blank again, so it makes sense that it has a default value. I hope this also makes it more clear that users can send less than 1 BTC if it shows the 8 decimal places
PR:
ACKs for top commit:
promag:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc.
GChuf:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc
laanwj:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc, this is a surprisingly compact solution too
Tree-SHA512: 354590d2a88231b8649f7ae985c8a7864d74ca0e1f8603cb1730ba46747084de90ee6285ce4d39ee04b054fb9cd2d78ebc71146f3af694c37a8a3aff7f051800
7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43 Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
e65e61c812df90a56e3ce4a8e76c4b746766f387 Add some general std::vector utility functions (Pieter Wuille)
Pull request description:
This is another general improvement extracted from #16800 .
Two functions are added are:
* Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization).
* Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant.
Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp
ACKs for top commit:
laanwj:
ACK 7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43
MarcoFalke:
ACK 7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43 (enjoyed reading the tests, but did not compile)
Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a
ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
----
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
eb7b78165966f2c79da71b993c4c4d793e37297f modify p2p_feefilter test to catch rounding error (Gregory Sanders)
6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
8e59af55aaf1b196575084bce2448af02d97d745 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)
Pull request description:
This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes.
Fixes https://github.com/bitcoin/bitcoin/issues/16499
Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500
ACKs for top commit:
ajtowns:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f code review only
naumenkogs:
utACK eb7b78165966f2c79da71b993c4c4d793e37297f
achow101:
re ACK eb7b78165966f2c79da71b993c4c4d793e37297f
promag:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f.
Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
5c5d0b62648e1b144b7b93c199f45265dac100e5 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
theStack:
Concept and code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
jamesob:
ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
faa86b71acefc8f2e366746a1c251888e6e686dd fuzz: Use ConsumeUInt256 helper to simplify rolling_bloom_filter fuzz test (MarcoFalke)
aaaa61fd306e25379e6222e31bf160a6eb04f74e fuzz: Speed up rolling_bloom_filter fuzz test (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log](https://github.com/bitcoin/bitcoin/files/6822159/clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log)
ACKs for top commit:
practicalswift:
cr ACK faa86b71acefc8f2e366746a1c251888e6e686dd
theStack:
Concept and code review ACK faa86b71acefc8f2e366746a1c251888e6e686dd
Tree-SHA512: eace588509dfddb2ba97baf86379fa713fa6eb758184abff676cb95807ff8ff36905eeaddeba05665b8464c35c57e2138f88caec71cbfb255e546bbe76558da0
faafda232e1d4f79ee64dbfee699a8018f25b0bc fuzz: Speed up prevector fuzz target (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35981
ACKs for top commit:
practicalswift:
cr ACK faafda232e1d4f79ee64dbfee699a8018f25b0bc
Tree-SHA512: 1bf166c4a99a8ce88bdc030cd6a32ce1da5251b73873772e0e9c001ec2bacafebb183f7c8c88806d0ab633aada2cff8b78791f5c9c0c6f2cc8ef5f0875c4b2ef
aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)
Pull request description:
Requested on IRC:
```
[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
ACKs for top commit:
jonatack:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 per `git diff fa74025 aaaa9c6`
vasild:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0
Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
fb7be92b094477131140b58a4e3ae98366b93e76 Mark print-% target as phony. (Dmitry Goncharov)
Pull request description:
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on force and mark force as phony.
This change ensures print-% rule works even when there is a file that matches the target.
```
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch force
$ make print-host
host=x86_64-pc-linux-gnu
```
ACKs for top commit:
hebasto:
ACK fb7be92b094477131140b58a4e3ae98366b93e76, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: b89ae66aa8c7aa6a7ab5f0956f9eb3b3ef9d56994b60dc2a97d498d4c1bba537845c190723e8a10310280b1b35df2cd935cc30aeb76735cac2dc621ad7823772
906d7913117c8f10934b37afa27ae8ac565da042 fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)
Pull request description:
It is possible to trigger a fuzztest failure in the `base_encode_decode` by asking it to decode any PSBT that has HD keypaths in it. For example, this one
```
cHNidP8BAFUCAAAAASeaIyOl37UfxF8iD6WLD8E+HjNCeSqF1+Ns1jM7XLw5AAAAAAD/////AaBa6gsAAAAAGXapFP/pwAYQl8w7Y28ssEYPpPxCfStFiKwAAAAAAAEBIJVe6gsAAAAAF6kUY0UgD2jRieGtwN8cTRbqjxTA2+uHIgIDsTQcy6doO2r08SOM1ul+cWfVafrEfx5I1HVBhENVvUZGMEMCIAQktY7/qqaU4VWepck7v9SokGQiQFXN8HC2dxRpRC0HAh9cjrD+plFtYLisszrWTt5g6Hhb+zqpS5m9+GFR25qaAQEEIgAgdx/RitRZZm3Unz1WTj28QvTIR3TjYK2haBao7UiNVoEBBUdSIQOxNBzLp2g7avTxI4zW6X5xZ9Vp+sR/HkjUdUGEQ1W9RiED3lXR4drIBeP4pYwfv5uUwC89uq/hJ/78pJlfJvggg71SriIGA7E0HMunaDtq9PEjjNbpfnFn1Wn6xH8eSNR1QYRDVb1GELSmumcAAACAAAAAgAQAAIAiBgPeVdHh2sgF4/iljB+/m5TALz26r+En/vykmV8m+CCDvRC0prpnAAAAgAAAAIAFAACAAAA=
```
which I took straight from the PSBT test vectors. The reason is that in src/psbt.h we call `DeserializeHDKeypaths`, which in turn calls `CPubKey::IsFullyValid`, which in turn asserts that a secp context has been created.
The error appears to be masked on many systems by the definition of `instance_of_eccryptoclosure` in src/script/bitcoinconsensus.cpp, which defines a static object which contains an `ECCVerifyHandle`. If you just comment out that line you can reliably trigger the fuzz test failure, e.g. by creating a file `crash` with the above PSBT, and runnnig
```
ASAN_OPTIONS=symbolize=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1 UBSAN_OPTIONS=suppressions=./test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1 FUZZ=base_encode_decode ./src/test/fuzz/fuzz -seed_inputs=crash
```
ACKs for top commit:
practicalswift:
cr ACK 906d7913117c8f10934b37afa27ae8ac565da042
Tree-SHA512: b98b60573c21efe28503fe351883c6f0d9ac99d0dd6f100537b16ac53476617b8a3f899faf0c23d893d34a01b3bbe4a784499ec6f9c7000292e850bed449bd85
9550dffa0c61df6d1591c62d09629b4c5731e1b7 fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
fa483e9f68b8b4171dabb25cc88dc2eada454a99 fuzz: Speed up crypto fuzz target (MarcoFalke)
Pull request description:
May fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34962
Similar solution to https://github.com/bitcoin/bitcoin/pull/22005
ACKs for top commit:
practicalswift:
cr ACK fa483e9f68b8b4171dabb25cc88dc2eada454a99: patch looks correct and rationale makes sense
Tree-SHA512: 3788cf9f6ba0f7a0a217cd3a6a825839689425e99e4d6d657981d291a001b0da7c5abb50a68b4ee1c2a8300b87fb92e4e3ccc1171907792b40251e467c33bd53
d44a261acff40c1c8727d3cc0106bde65a6416d0 Fix issues when `walletdir` is root directory (unknown)
Pull request description:
+ Remove one character less from wallet path
+ After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR https://github.com/bitcoin/bitcoin/pull/21907 helped me resolve the issue.
**Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed
**Solution**: `if` statement to check `walletdir` is a root directory
Fixes: https://github.com/bitcoin/bitcoin/issues/21510https://github.com/bitcoin/bitcoin/issues/21501
Related PR: https://github.com/bitcoin/bitcoin/pull/20080
Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`:
Before this PR:
```
{
"wallets": [
{
"name": "1"
},
{
"name": "2"
}
]
}
```
After this PR:
```
"wallets": [
{
"name": "w1"
},
{
"name": "w2"
}
]
}
```
ACKs for top commit:
ryanofsky:
Code review ACK d44a261acff40c1c8727d3cc0106bde65a6416d0
meshcollider:
utACK d44a261acff40c1c8727d3cc0106bde65a6416d0
Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55
1be6267ce1ee142c3b90baed1925a82eab6514aa fuzz: don't try and use fopencookie when building for Android (fanquake)
Pull request description:
When building for Android, `_GNU_SOURCE` will be defined:
```bash
/home/ubuntu/android-sdk/ndk/22.1.7171670/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android30-clang++ -dM -E -x c++ - < /dev/null
#define _GNU_SOURCE 1
#define _LP64 1
#define __AARCH64EL__ 1
#define __AARCH64_CMODEL_SMALL__ 1
#define __ANDROID_API__ 30
#define __ANDROID__ 1
#define __ARM_64BIT_STATE 1
.....
```
but it doesn't have the [`fopencookie()` function](https://www.gnu.org/software/libc/manual/html_node/Streams-and-Cookies.html), or define the `cookie_io_functions_t` type, which results in compile failures:
```bash
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:388:15: error: unknown type name 'cookie_io_functions_t'
const cookie_io_functions_t io_hooks = {
^
15 warnings and 1 error generated.
```
Just skip trying to use it if we are building for Android. Should fix#22062.
ACKs for top commit:
practicalswift:
cr ACK 1be6267ce1ee142c3b90baed1925a82eab6514aa
Tree-SHA512: d62f63d0624af04b76c7e07b0332c71eca2bf9cd9e096a60aea9e212b7bbc1548e9fa9a76d065ec719bb345fe8726619c3bd2d0631f54d877c82972b7b289321
5e146022daa4336de94447e5b8e5418296286927 wallet: fix scanning progress calculation for single block range (Sebastian Falbesoner)
Pull request description:
If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the `getwalletinfo` RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes#20297.
The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via
```bash
#!/bin/bash
while true
do
bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done
```
and at the same time perform some `getwalletinfo` RPCs.
On the master branch, this leads to frequent invalid responses (tested on mainchain):
```
$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}
```
(note that missing value for "progress" in the JSON result).
On the PR branch, the behaviour doesn't occur anymore.
ACKs for top commit:
MarcoFalke:
review ACK 5e146022daa4336de94447e5b8e5418296286927
promag:
Core review ACK 5e146022daa4336de94447e5b8e5418296286927.
Tree-SHA512: f0e6aad5a6cd08b36c5fe820fff0ef26663229b39169a4dbe757f3c795a41cf5c69c9dc90efe7515675ae1059307f8971123781a0514d10704123a6f28b125ab
## Issue being fixed or feature implemented
Bad coding in governance code
## What was done?
Use Enums where possible
## How Has This Been Tested?
Make check
## Breaking Changes
none
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Upgraded version of cppcheck
## What was done?
## How Has This Been Tested?
Ran cppcheck
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Develop doesn't build: ./autogen.sh fails
## What was done?
remove newline
## How Has This Been Tested?
./autogen.sh
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Converts some CCriticalSections with Mutexes; other minor refactoring
in
0fce09d1f0
see before
<img width="771" alt="image"
src="https://user-images.githubusercontent.com/6443210/225969163-bb4cee62-3e6a-4224-980a-11b2e0024a60.png">
and after
<img width="766" alt="image"
src="https://user-images.githubusercontent.com/6443210/225969245-e8afcbf6-c112-40c4-9504-82830b005a53.png">
## What was done?
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
fa18553d382a7d8c447cd6698b36e293fb7ecf1f fuzz: Remove addrdb fuzz target (MarcoFalke)
Pull request description:
The target has several issues:
* It is named incorrectly (`addrdb`, but it constructs a `CBanEntry`)
* It doesn't do anything meaningful, other than consuming one integer and passing it to a constructor
* It consumes CPU time that can be used for the other targets
* It is redundant with the banman fuzz target
Fix all by removing it.
ACKs for top commit:
amitiuttarwar:
ACK fa18553d382a7d8c447cd6698b36e293fb7ecf1f, thanks for the cleanup
Tree-SHA512: 3f8944d3f80913bf466c03062fed070e96073fb72d0938b2bc9a2586960c86879d6f251e16fd81cfeb4e6685ff9eef6bccb25cd3901b218a100c90f25a3c9240
14e8cf974a7a317796ef8e97e5cf9c355ceff0ee [consensus] MOVEONLY: Move single-sig checking EvalScript code to EvalChecksig (Pieter Wuille)
Pull request description:
This is another small refactor pulled out of the Schnorr/Taproot PR #17977.
This is in preparation for adding different signature verification rules,
specifically tapscript (BIP 342), which interprets opcode 0xac and 0xad
as Schnorr signature verifications.
ACKs for top commit:
sipa:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, verified move-only.
MarcoFalke:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, reviewed with "git show 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space -W" 👆
fjahr:
Code-review ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, verified that it's move-only.
instagibbs:
code review ACK 14e8cf974a, verified move-only
theStack:
Code-Review ACK 14e8cf974a
jonatack:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee
Tree-SHA512: af2efce9ae39d5ec01db5b9ef0ff383fe252ef5f33b3483927308ae17d91a619266cb45951f32ea1ce54807a4c0f052bcdefb47e244465d3a726393221c227b1
13076867981ab36b3549ab4c29583ae8ed12a709 refactor: Use Mutex type for g_cs_recent_confirmed_transactions (Hennadii Stepanov)
Pull request description:
No need the `RecursiveMutex` type for the `g_cs_recent_confirmed_transactions`.
Related to #19303.
ACKs for top commit:
MarcoFalke:
ACK 13076867981ab36b3549ab4c29583ae8ed12a709
vasild:
ACK 13076867
Tree-SHA512: 67f1be10c80ec18d0f80b9f5036e5a20986314da9b9364ef4e193ad1d9f3f4c8e4c2e16253ca79d649ff602d5b8c2aff58d7dd1085841afb760479a4875cffbe
37ae687f95c82f2d64ed880533d158060d4fc3de Add tests for CPubKey serialization/unserialization (Elichai Turkel)
9b8907faded8e4ec312c0dd4b4b15e1793876acd Check size after Unserializing CPubKey (Elichai Turkel)
Pull request description:
Found by practicalswift, closes#19235
Currently all the public API(except the pointer-like API) in CPubKey that sets/constructs a pubkey goes through `CPubKey::Set` which checks if that the length and size match and if not invalidates the key.
This adds the same check to `CPubKey::Unserialize`, sadly I don't see an easy way to just push this to the existing checks in `CPubKey::Set` but it's only a simple condition.
The problem with not invalidating is that if you write a pubkey like: `{0x02,0x00}` it will think the actual length is 33(because of `size()`) and will access uninitialized memory if you call any of the functions on CPubKey.
ACKs for top commit:
practicalswift:
re-ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
jonatack:
Code review re-ACK 37ae687 per `git diff eab8ee3 37ae687` only change since last review at eab8ee3 is passing the `pubkey` param by reference to const instead of by value in `src/test/key_tests.cpp::CmpSerializationPubkey`
MarcoFalke:
ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1
