725d7ae049 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd6 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507 Add MissingDataBehavior and make TransactionSignatureChecker handle it (Pieter Wuille)
Pull request description:
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
* P2WPKH/P2WSH hashes need the spent amount
* P2TR hashes need all spent outputs (amount + scriptPubKey)
Missing amounts are treated as -1 (thus leading to unexpected signature failures), while missing outputs in P2TR validation cause assertion failure. This is hard to extend for signing support, and also quite ugly in general.
In this PR, an explicit configuration option to {Mutable,}TransactionSignatureChecker is added (MissingDataBehavior enum class) to either select ASSERT_FAIL or FAIL. Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL.
The existence of the ASSERT_FAIL option is really just an abundance of caution. Always using FAIL should be just fine, but if there were for some reason a code path in consensus code was introduced that misses certain data, I think we prefer as assertion failure over silently introducing a consensus change.
Potentially useful follow-ups (not for this PR, in my preference):
* Having an explicit script validation error code for missing data.
* Having a MissingDataBehavior::SUCCEED option as well, for use in script/sign.cpp DataFromTransaction (if a signature is present in a witness, and we don't have enough data to fully validate it, we should probably treat it as valid and not touch it).
ACKs for top commit:
sanket1729:
reACK 725d7ae049
Sjors:
ACK 725d7ae049
achow101:
re-ACK 725d7ae049
benthecarman:
ACK 725d7ae049
fjahr:
Code review ACK 725d7ae049
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
Since we want tests to run quickly, and since tests do a lot more db
operations than expected we expect to see in actual usage, we disable
sqlite's syncing behavior to make db operations run much faster. This
syncing behavior is necessary for normal operation as it helps guarantee
that data won't become lost or corrupted, but in tests, we don't care
about that.
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.
This generalises the ComputeBlockVersion test so that it can apply to
any activation parameters we might set, and checks all the parameters
set for each deployment on each chain, to simultaneously ensure that the
deployments we have configured work sensibly, and that the test code
does not suffer bitrot in the event that all interesting deployments
are buried.
The intent here is to allow checking ComputeBlockVersion behaviour with
each deployment, rather than only testdummy on mainnet. This commit does
the trivial refactoring component of that change.
d3b0b08b0f doc: release notes for new listbanned fields (Jarol Rodriguez)
60290d3f5e test: increase listbanned unit test coverage (Jon Atack)
3e978d1a5d rpc: add time_remaining field to listbanned (Jarol Rodriguez)
5456b34531 rpc: add ban_duration field to listbanned (Jarol Rodriguez)
c95c61657a doc: improve listbanned help (Jarol Rodriguez)
dd3c8eaa33 rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez)
Pull request description:
This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields
It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](https://github.com/bitcoin/bitcoin/pull/21602#issuecomment-813486134) by jonatack, `time_remaining` is another useful user-centric data point.
Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical.
This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields.
**Master: listbanned**
```
[
{
"address": "1.2.3.4/32",
"banned_until": 1617691101,
"ban_created": 1617604701
},
{
"address": "135.181.41.129/32",
"banned_until": 1649140716,
"ban_created": 1617604716
}
]
```
**PR: listbanned**
```
[
{
"address": "1.2.3.4/32",
"ban_created": 1617775773,
"banned_until": 1617862173,
"ban_duration": 86400,
"time_remaining": 86392
},
{
"address": "3.114.211.172/32",
"ban_created": 1617753165,
"banned_until": 1618357965,
"ban_duration": 604800,
"time_remaining": 582184
}
]
```
ACKs for top commit:
jonatack:
re-ACK d3b0b08b0f
hebasto:
ACK d3b0b08b0f, tested on Linux Mint 20.1 (x86_64).
MarcoFalke:
review ACK d3b0b08b0f🕙
Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
faaf3954e2 fuzz: Extend psbt fuzz target a bit (MarcoFalke)
Pull request description:
Previously it only merged the psbt with itself, now it tries to merge another.
ACKs for top commit:
practicalswift:
Tested ACK faaf3954e2
Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
fa6183d776 test: Remove option to make TestChain100Setup non-deterministic (MarcoFalke)
fa732bccb3 test: Use compressed keys in TestChain100Setup (MarcoFalke)
Pull request description:
Seems odd to have an option for non-deterministic tests
when the goal should be for all tests to be deterministic.
ACKs for top commit:
jamesob:
ACK fa6183d776
practicalswift:
cr ACK fa6183d776: patch looks deterministic!
Tree-SHA512: 6897a9f36e0dfb7d63b25dd6984414b3ee8a62458ad232cb21ed5077184fdb0bc626996e4ac84ef0bdd452b9f17c54aac75a71575b8e723b84cac07c9f9d5611
867a5e172a guix: Register garbage collector root for containers (Carl Dong)
8f8b96fb54 guix: Update hint messages to mention guix-clean (Carl Dong)
44f6d4f56b guix: Record precious directories and add guix-clean (Carl Dong)
84912d4b24 build: Remove spaces from variable-printing rules (Carl Dong)
Pull request description:
```
guix: Record precious directories and add guix-clean
Many users have reported problems that stem from having an unclean
working tree. To that end, I've written a guix-clean script which should
help reset the working tree while respecting user-specified precious
directories.
Precious directories, such as:
- SOURCES_PATH
- BASE_CACHE
- SDK_PATH
- OUTDIR
Should be preserved when cleaning the working tree, and are thus
recorded in ./contrib/guix/var/precious_dirs.
The ./contrib/guix/guix-clean script is able to parse that file and make
sure to avoid them when cleaning out the working tree.
```
ACKs for top commit:
laanwj:
ACK 867a5e172a
Tree-SHA512: c498fad781ff5e6406639df2b91b687fc528273fdf266bcdba8f6eec3b3b37ecce544b6da0252f0b9c6717f9d88e844e4c7b72d1877bdbabfc6871ddd0172af5
This commit moves the ExternalSigner class and RPC methods out of the wallet module.
The enumeratesigners RPC can be used without a wallet since #21417.
With additional modifications external signers could be used without a wallet in general, e.g. via signrawtransaction.
The signerdisplayaddress RPC is ranamed to walletdisplayaddress because it requires wallet context.
A future displayaddress RPC call without wallet context could take a descriptor argument.
This commit fixes a rpc_help.py failure when configured with --disable-wallet.
9044522ef7 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky)
Pull request description:
This just makes an additional simplification after #21366 replaced
util::Ref with std::any. It was originally suggested
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351 but
delayed for a followup. It would have prevented usage bug
https://github.com/bitcoin/bitcoin/pull/21572.
ACKs for top commit:
promag:
ACK 9044522ef7, fixed conflict in src/wallet/interfaces.cpp.
Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
Seems odd to have an option for non-deterministic tests
when the goal should be for all tests to be deterministic.
Can be reviewed with `--ignore-all-space`.
There is no way to iterate through all script verification flags, and
it's not guaranteed that every power of 2 is used. Just make sure that
all flags in STANDARD_SCRIPT_VERIFY_FLAGS are present in mapFlagNames;
this covers all consensus and policy flags. If mapFlagNames has more
flags than STANDARD_SCRIPT_VERIFY_FLAGS, that's okay. Nonexistent flags
will be caught by the compiler.
Similar to 19db590d04, which removed these
for the valid tests. Not removing ones that cause a false/empty stack
error because these tests should fail due to being invalid with CSV/CLTV
Add missing script verify flags to mapFlagNames.
iterate through mapFlagNames values instead of bits.
BOOST_CHECK_MESSAGE better reports which test failed exactly, whereas
BOOST_ERROR was just incrementing the error counter.
5c446784b1 rpc: improve getnodeaddresses help (Jon Atack)
1b9189866a rpc: simplify/constify getnodeaddresses code (Jon Atack)
3bb6e7b655 rpc: add network field to rpc getnodeaddresses (Jon Atack)
Pull request description:
This patch adds a network field to RPC `getnodeaddresses`, which is useful on its own, particularly with the addition of new networks like I2P and others in the future, and which I also found helpful for adding a new CLI command as a follow-up to this pull that calls `getnodeaddresses` and needs to know the network of each address.
While here, also improve the `getnodeaddresses` code and help.
```
$ bitcoin-cli -signet getnodeaddresses 3
[
{
"time": 1611564659,
"services": 1033,
"address": "2600:1702:3c30:734f:8f2e:744b:2a51:dfa5",
"port": 38333,
"network": "ipv6"
},
{
"time": 1617531931,
"services": 1033,
"address": "153.126.143.201",
"port": 38333,
"network": "ipv4"
},
{
"time": 1617473058,
"services": 1033,
"address": "nsgyo7begau4yecc46ljfecaykyzszcseapxmtu6adrfagfrrzrlngyd.onion",
"port": 38333,
"network": "onion"
}
]
$ bitcoin-cli help getnodeaddresses
getnodeaddresses ( count )
Return known addresses, which can potentially be used to find new nodes in the network.
Arguments:
1. count (numeric, optional, default=1) The maximum number of addresses to return. Specify 0 to return all known addresses.
Result:
[ (json array)
{ (json object)
"time" : xxx, (numeric) The UNIX epoch time when the node was last seen
"services" : n, (numeric) The services offered by the node
"address" : "str", (string) The address of the node
"port" : n, (numeric) The port number of the node
"network" : "str" (string) The network (ipv4, ipv6, onion, i2p) the node connected through
},
...
]
```
Future idea: allow passing `getnodeaddresses` a network (or networks) as an argument to return only addresses in that network.
ACKs for top commit:
laanwj:
Tested ACK 5c446784b1
jarolrod:
re-ACK 5c446784b1
promag:
Code review ACK 5c446784b1.
Tree-SHA512: ab0101f50c76d98c3204133b9f2ab6b7b17193ada31455ef706ad11afbf48f472fa3deb33e96028682369b35710ccd07d81863d2fd55c1485f32432f2b75efa8
937fd4a66f Fix wrong wallet RPC context set after #21366 (Russell Yanofsky)
Pull request description:
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking. Suggested
change
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
eliminates these and would be a good followup for a future PR.
This PR just implements the simplest possible fix.
ACKs for top commit:
theStack:
Code-review ACK 937fd4a66f
meshcollider:
Code review ACK 937fd4a66f
Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
a4e970adb6 build: enable -Wdocumentation if suppressing external warnings (fanquake)
3b0078f958 doc: fixup -Wdocumentation issues (fanquake)
c6edcf1c71 build: suppress libevent warnings if supressing external warnings (fanquake)
Pull request description:
Enable `-Wdocumentation` by taking advantage of our `--enable-suppress-external-warnings` flag. Most of the CIs are using this flag now, so any regressions should be caught.
This also required modifying libevents flags when suppressing warnings, as depending on the version being built against, that could generate a large number of warnings. i.e:
```bash
In file included from httpserver.cpp:34:
In file included from ./support/events.h:12:
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:464:11: warning: parameter 'req' not found in the function declaration [-Wdocumentation]
@param req a request object
^~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:465:11: warning: parameter 'databuf' not found in the function declaration [-Wdocumentation]
@param databuf the data chunk to send as part of the reply.
^~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:467:11: warning: parameter 'call' not found in the function declaration [-Wdocumentation]
@param call back's argument.
^~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:939:4: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated; you probably want to use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:946:1: note: add a deprecation attribute to the declaration to silence this warning
char *evhttp_decode_uri(const char *uri);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:979:5: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated as of Libevent 2.0.9. Use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:987:1: note: add a deprecation attribute to the declaration to silence this warning
int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: warning: parameter 'query_parse' not found in the function declaration [-Wdocumentation]
@param query_parse the query portion of the URI
^~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: note: did you mean 'uri'?
@param query_parse the query portion of the URI
^~~~~~~~~~~
uri
69 warnings generated.
```
Note that a lot of these have already been fixed upstream.
ACKs for top commit:
laanwj:
Concept and code review ACK a4e970adb6
practicalswift:
cr ACK a4e970adb6: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback
jonatack:
Light ACK a4e970adb6 skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted
Tree-SHA512: 57a1e30cffcc8bcceee72d85f58ebe29eae525861c70acb237541bd480c51ede89875c033042c0af376fdbb49fb7f588ef9282a47c6e78f9d4501c41f1b21eb6
ea19cc844e wallet: refactor: dedup sqlite statement deletions (Sebastian Falbesoner)
9a3670930e wallet: refactor: dedup sqlite statement preparations (Sebastian Falbesoner)
Pull request description:
This refactoring PR deduplicates repeated SQLite statement preparation calls (`sqlite3_prepare_v2(...)`) / deletions (`sqlite3_finalize(...)`) and its surrounding logic by putting each prepared statement and its corresponding text representation into a ~std::map~ ~`std::array`~ `std::vector`. This should be more readable and less error-prone, e.g. in case an additional statement needs to be added in the future or the error handling has to be adapted.
ACKs for top commit:
achow101:
ACK ea19cc844e
meshcollider:
utACK ea19cc844e
Tree-SHA512: ced89869b2147e088e7a4cda2acbbdd4a806f66dbc2d6999953d0d702c0655aa53c0eb699cc7e5e3732f2d24206d577a9d9e1b5de7f439100dead2696ade1092
fa9b74f5ea Fix assumeutxo crash due to missing base_blockhash (MarcoFalke)
fa8fffebe8 refactor: Prefer clean assert over UB in coinstats (MarcoFalke)
Pull request description:
This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:
* Adds an `Assert` to transform the UB into a clean crash, even when sanitizers are disabled
* Adds an early-fail condition to avoid the crash
ACKs for top commit:
jamesob:
ACK fa9b74f5ea ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))
ryanofsky:
Code review ACK fa9b74f5ea with no code changes since last review, just splitting up combocommit a little.
Tree-SHA512: dd36808a09f49c647543a9eaa6fdb785b3f1109af48ba4cc983153b22a144da9ca61af22034dcfaa0e192a65b1ee7de744f187555079aff55bec0efa0ce87cd4
a97a9298ce Test that signrawtx works when a signed CSV and CLTV inputs are present (Andrew Chow)
6965456c10 Introduce DeferringSignatureChecker and inherit with SignatureExtractor (Andrew Chow)
Pull request description:
Previously SignatureExtractorChecker took a MutableTransactionSignatureChecker and passed through function calls to that. However not all functions were implemented so not everything passed through as it should have. To solve this, SignatureExctractorChecker now implements all of those functions via a new class - DeferredSignatureChecker. DeferredSignatureChecker is introduced to allow for future signature checkers which use another SignatureChecker but need to be able to do somethings outside of just the signature checking.
Fixes#21151
ACKs for top commit:
sipa:
utACK a97a9298ce
meshcollider:
Code review ACK a97a9298ce
instagibbs:
utACK a97a9298ce
Tree-SHA512: bca784c75c2fc3fcb74e81f4e3ff516699e8debaa2db81e12843abdfe9cf265dac11db8619751cb9b3e9bbe779805d029fabe5f3cbca5e86bfd72de3664b0b94
b2ee8b207d net: Deserialize hardcoded seeds from BIP155 blob (W. J. van der Laan)
9b29d5df7f contrib: Add explicit port numbers for testnet seeds (W. J. van der Laan)
2a257de113 contrib: Add a few TorV3 seed nodes (W. J. van der Laan)
06030f7a42 contrib: generate-seeds.py generates output in BIP155 format (W. J. van der Laan)
Pull request description:
Closes#20239 and mitigates my node's problem in #21351.
- Add a few hardcoded seeds for TorV3
- As the [bitcoin-seeder](https://github.com/sipa/bitcoin-seeder) doesn't collect TorV3 addresses yet, I have extracted these from my own node using [a script](https://gist.github.com/laanwj/b3d7b01ef61ce07c2eff0a72a6b90183) and added them manually. This is intended to be a temporary stop gap until 22.0's seeds update.
- Change hardcoded seeds to variable length BIP155 binary format.
- It is stored as a single serialized blob in a byte array, instead of pseudo-IPv6 address slots. This is more flexible and, assuming most of the list is IPv4, more compact.
- Only the (networkID, addr, port) subset (CService). Services and time are construed on the fly as before.
- Change input format for `nodes_*.txt`.
- Drop legacy `0xAABBCCDD` format for IPv4. It is never generated by `makeseeds.py`.
- Stop interpreting lack of port as default port, interpret it as 'no port', to accomodate I2P and other port-less protocols (not handled in this PR). An explicit port is always generated by `makeseeds.py` so in practice this makes no difference right now.
A follow-up to this PR could do the same for I2P.
ACKs for top commit:
jonatack:
ACK b2ee8b207d
Tree-SHA512: 11a6b54f9fb0192560f2bd7b218f798f86c1abe01d1bf37f734cb88b91848124beb2de801ca4e6f856e9946aea5dc3ee16b0dbb9863799e42eec1b239d40d59d
81747b2171 test: make sure non-IP peers get discouraged and disconnected (Vasil Dimov)
637bb6da36 test: also check disconnect in denialofservice_tests/peer_discouragement (Vasil Dimov)
4d6e246fa4 test: use pointers in denialofservice_tests/peer_discouragement (Vasil Dimov)
Pull request description:
Split up from #20966, so that it can be backported easier. Merging this ahead of #20966 will also reduce the number of conflicts for that pull.
ACKs for top commit:
jonatack:
ACK 81747b2171
Tree-SHA512: 8f0e30b95baba7f056920d7fc3b37bd49ee13e69392fe80e2d333c6bb09fd25f4603249301b8795cca26a2f2d15b9f8904798a55cd9c04fd28afb316e95c551c
The doc nicely explains why the directory exists and it is
irrelevant when it was introduced. Even if it was relevant,
it could be trivially found out via `git log ./src/node/ | tail`
without visiting GitHub
fa5eabe721 refactor: Remove negative lock annotations from globals (MarcoFalke)
Pull request description:
They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional.
ACKs for top commit:
sipa:
utACK fa5eabe721
ajtowns:
ACK fa5eabe721
hebasto:
ACK fa5eabe721
vasild:
ACK fa5eabe721
Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc
fa8192f42e rpc: Fail to return undocumented return values (MarcoFalke)
Pull request description:
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476
Fix this by treating it as an internal bug to return undocumented return values.
ACKs for top commit:
ryanofsky:
Code review ACK fa8192f42e. Only changes: rebase, no const_cast suggestion, and tostring cleanups needed after suggestion
Tree-SHA512: c006905639bafe3045de152b00c34d9864731becb3c4f468bdd61a392f10d7e7cd89a54862c8daa8c11ac4eea0eb5f13b0f647d21e21a0a797b54191cff7238c
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking. Suggested
change
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
eliminates these and would be a good followup for a future PR.
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.
The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.
If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
2e5f7def22 wallet, rpc: update listdescriptors response format (Ivan Metlushko)
Pull request description:
Update `listdescriptors` response format according to [RPC interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines).
This is a follow up for #20226
**Before:**
```
Result:
[ (json array) Response is an array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
```
**After:**
```
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"descriptors" : [ (json array) Array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
}
```
ACKs for top commit:
achow101:
re-ACK 2e5f7def22
meshcollider:
utACK 2e5f7def22
jonatack:
re-ACK 2e5f7def22
Tree-SHA512: 49bf73e46e2a61003ce594a4bfc506eb9592ccb799c2909c43a1a527490a4b4009f78dc09f3d47b4e945d3d7bb3cd2632cf48c5ace5feed5066158cc010dddc1
4eca20d6f7 [doc] correct comment about ATMPW (glozow)
8fa74aeb5b [doc] correct comment in chainparams (glozow)
2f8272c2a4 [doc] GetBestBlock() doesn't do nothing (gzhao408)
Pull request description:
Came across a few misleading comments, wanted to fix them
ACKs for top commit:
jnewbery:
ACK 4eca20d6f7
MarcoFalke:
ACK 4eca20d6f7
laanwj:
Code review ACK 4eca20d6f7
Tree-SHA512: 5bef1f1e7703f304128cf0eb8945e139e031580c99062bbbe15bf4db8443c2ba5a8c65844833132e6646c8980c678fc1d2ab0c63e17105585d583570ee350fd0
73e1f7d754 rpc: document optional fields for getchaintxstats result (Sebastian Falbesoner)
Pull request description:
This mini-PR updates the result help of the `getchaintxstats` RPC by showing the following fields as "optional":
- window_tx_count
- window_interval
- txrate
Help output diff between master and PR branch:
```diff
16,18c16,18
< "window_tx_count" : n, (numeric) The number of transactions in the window. Only returned if "window_block_count" is > 0
< "window_interval" : n, (numeric) The elapsed time in the window in seconds. Only returned if "window_block_count" is > 0
< "txrate" : n (numeric) The average rate of transactions per second in the window. Only returned if "window_interval" is > 0
---
> "window_tx_count" : n, (numeric, optional) The number of transactions in the window. Only returned if "window_block_count" is > 0
> "window_interval" : n, (numeric, optional) The elapsed time in the window in seconds. Only returned if "window_block_count" is > 0
> "txrate" : n (numeric, optional) The average rate of transactions per second in the window. Only returned if "window_interval" is > 0
```
ACKs for top commit:
0xB10C:
ACK 73e1f7d754
Tree-SHA512: 63c8db3e47a3c2d5564d53c564484b95b656e1e5deca1e9841bc90d122d3c81f02fd2b59313fd913ce81b16f7cc2969fe1dd9d6c3e23628b8ac057ea08f55daa
1111896eb7 doc: Merge release notes (MarcoFalke)
faeba9819d rpc: Missing doc updates for bumpfee psbt update (MarcoFalke)
Pull request description:
Stuff missed in #20891. Also merge release notes, so that it doesn't have to be done later.
ACKs for top commit:
fanquake:
ACK 1111896eb7
Tree-SHA512: c9be5a3c944e2981c83546c4761277f1ad5fb9ba97bec80d073db4229924cb48fd23cb5638217c844e05af51d80507718dd201099cbe50819986b3c47c5df7e5
935d488922 [net processing] Refactor MaybeSendAddr() (John Newbery)
01a79ff924 [net processing] Fix overindentation in MaybeSendAddr() (John Newbery)
38c0be5da3 [net processing] Refactor MaybeSendAddr() - early exits (John Newbery)
c87423c58b [net processing] Change MaybeSendAddr() to take a reference (John Newbery)
ad719297f2 [net processing] Extract `addr` send functionality into MaybeSendAddr() (John Newbery)
4ad4abcf07 [net] Change addr send times fields to be guarded by new mutex (John Newbery)
c02fa47baa [net processing] Only call GetTime() once in SendMessages() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing. It refactors `addr` send functionality into its own function `MaybeSendAddr()` and flattens/simplifies the code. Isolating and simplifying the addr handling code makes subsequent changes (which will move addr data and logic into net processing) easier to review.
This is a pure refactor. There are no functional changes.
For motivation of the project, see #19398.
ACKs for top commit:
sipa:
utACK 935d488922
hebasto:
ACK 935d488922, I have reviewed the code and it looks OK, I agree it can be merged.
MarcoFalke:
review ACK 935d488922🐑
Tree-SHA512: 4e9dc84603147e74f479a211b42bcf315bdf5d14c21c08cf0b17d6c252775b90b012f0e0d834f1a607ed63c7ed5c63d5cf49b134344e7b64a1695bfcff111c92
Throwing an exception from a slot invoked by Qt's signal-slot connection
mechanism is considered undefined behavior, unless it is handled within
the slot. The GUIUtil::ExceptionSafeConnect function should be used for
exception handling within slots.
916ab0195d remove unused class util::Ref and its unit test (Sebastian Falbesoner)
8dbb87a393 refactor: replace util::Ref by std::any (C++17) (Sebastian Falbesoner)
95cccf8a4b util: introduce helper AnyPtr to access std::any instances (Sebastian Falbesoner)
Pull request description:
As described in `util/ref.h`: "_This implements a small subset of the functionality in C++17's std::any class, and **can be dropped when the project updates to C++17**_". For accessing the contained object of a `std::any` instance, a helper template function `AnyPtr` is introduced (thanks to ryanofsky).
ACKs for top commit:
hebasto:
re-ACK 916ab0195d, with command
ryanofsky:
Code review ACK 916ab0195d. Changes since last review: rebase and replacing types with `auto`. I might have used `const auto*` and `auto*` instead of plain `auto` because I think the qualifiers are useful, but this is all good.
Tree-SHA512: fe2c3e4f5726f8ad40c61128339bb24ad11d2c261f71f7b934b1efe3e3279df14046452b0d9b566917ef61d5c7e0fd96ccbf35ff810357e305710f5002c27d47
Changes to make MaybeSendAddr simpler and easier to maintain/update:
- assert invariant that node.vAddrToSend.size() can never exceed
MAX_ADDR_TO_SEND
- erase known addresses from vAddrToSend in one pass
- no check for (vAddr.size() >= MAX_ADDR_TO_SEND) during iteration,
since vAddr can never exceed MAX_ADDR_TO_SEND.
4783115fd4 net: add ifaddrs.h include (fanquake)
879215e665 build: check if -lsocket is required with *ifaddrs (fanquake)
87deac66aa rand: only try and use freeifaddrs if available (fanquake)
Pull request description:
Fixes#21485 by linking against `-lsocket` when it's required for using `*ifaddrs` functions.
ACKs for top commit:
laanwj:
Code review ACK 4783115fd4
hebasto:
ACK 4783115fd4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4542e036e9b029de970eff8a9230fe45d9204bb22313d075f474295d49bdaf1f1cbb36c0c6e2fa8dbbcdba518d8d3a68a6116ce304b82414315f333baf9af0e4
fa4926cca6 fuzz: [refactor] Use IsValidFlagCombination in signature_checker fuzz target (MarcoFalke)
eeee8f5be1 fuzz: Removed unused try-catch in coins_view (MarcoFalke)
fa98f3f66e fuzz: [refactor] Use ConsumeScript in signature_checker fuzz target (MarcoFalke)
Pull request description:
Some small refactors to remove unused and redundant fuzz code
ACKs for top commit:
practicalswift:
cr re-ACK fa4926cca6
Tree-SHA512: eb07a2140caad7b31495b76385fc7634cf5b6daa4947f430ebb127eb1375583dc11e541a0a42d0e5d93d430480b8a815b93974450fd5ed897528a2d47c752f86
40316a37cb test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77970 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a8 net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d01 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)
Pull request description:
Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.
Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in https://github.com/bitcoin/bitcoin/pull/21407.
ACKs for top commit:
practicalswift:
Tested ACK 40316a37cb
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb reviewed `git range-diff 01bb3afb 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
laanwj:
Code review ACK 40316a37cb
Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
0cca08a8ee Add unit test coverage for our onion peer eviction protection (Jon Atack)
caa21f586f Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack)
8f1a53eb02 Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack)
8b1e156143 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack)
72e30e8e03 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack)
ca63b53ecd Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack)
41f84d5ecc Move peer eviction tests to a separate test file (Jon Atack)
f126cbd6de Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack)
Pull request description:
Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500.
Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in https://github.com/bitcoin/bitcoin/pull/20197#issuecomment-713865992.
This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime.
This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477.
Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
Closes#11537.
ACKs for top commit:
laanwj:
Code review ACK 0cca08a8ee
vasild:
ACK 0cca08a8ee
Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd
3fc06d3d7b [net] remove fUpdateConnectionTime from FinalizeNode (John Newbery)
7c4cc67c0c [net] remove CConnman::AddNewAddresses (John Newbery)
bcd7f30b79 [net] remove CConnman::MarkAddressGood (John Newbery)
8073673dbc [net] remove CConnman::SetServices (John Newbery)
392a95d393 [net_processing] Keep addrman reference in PeerManager (John Newbery)
1c25adf6d2 [net] Construct addrman outside connman (John Newbery)
Pull request description:
Addrman is currently a member variable of connman. Make it a top-level component with lifetime owned by node.context, and add a reference to addrman in peerman. This allows us to eliminate some functions in connman that are simply forwarding requests to addrman, and simplifies the connman-peerman interface.
By constructing the addrman in init, we can also add parameters to the ctor, which allows us to test it better. See #20233, where we enable consistency checking for addrman in our functional tests.
ACKs for top commit:
MarcoFalke:
re-ACK 3fc06d3d7b only change is squash 🏀
vasild:
ACK 3fc06d3d7b
Tree-SHA512: 17662c65cbedcd9bd1c194914bc4bb4216f4e3581a06222de78f026d6796f1da6fe3e0bf28c2d26a102a12ad4fbf13f815944a297f000e3acf46faea42855e07
This changes all context dependent checks in the parser to be
disjunctions of equality checks, rather than also including inequalities.
This makes sure that adding a new context enum in the future won't change
semantics for existing checks.
The error messages are also made a bit more consistent.
This is a preparation for parsing xonly pubkeys, which will complicate
this logic. It's cleaner to put the decision logic close to the public
key parsing itself.
This has no effect for now, as the only fragments with sub-script
expressions (sh, wsh) only allow one, and don't have key expressions
in them.
A future Taproot descriptor will however violate both, and we want
the keys in different sub-scripts to be assigned non-overlapping
cache indices.
So far, no descriptor exists that supports more than one sub-script
descriptor. This will change with taproot, so prepare for this by
changing the m_subdescriptor_arg from a unique_ptr to a vector of
unique_ptr's.
There are currently two DescriptorImpl subclasses that rely on the functionality
that ExpandHelper automatically adds subscripts to the output SigningProvider.
Taproot descriptors will have subscripts, but we don't want them in the
SigningProvider's bare script field. To avoid them ending up there, move this
functionality into the specific classes' MakeScripts implementation.
90ae3d8ca6 doc: Add release notes for -deprecatedrpc=addresses and bitcoin-tx (Michael Dietz)
085b3a7299 rpc: deprecate `addresses` and `reqSigs` from rpc outputs (Michael Dietz)
Pull request description:
Considering the limited applicability of `reqSigs` and the confusing output of `1` in all cases except bare multisig, the `addresses` and `reqSigs` outputs are removed for all rpc commands.
1) add a new sane "address" field (for outputs that have an identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact (with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely always.
Note: Some light refactoring done to allow us to very easily delete a few chunks of code (marked with TODOs) when we remove this deprecated behavior.
Using `IsDeprecatedRPCEnabled` in core_write.cpp caused some circular dependencies involving core_io
Circular dependencies were caused by rpc/util unnecessarily importing node/coinstats and node/transaction. Really what rpc/util needs are some fundamental type/helper-function definitions. So this was cleaned up to make more sense.
This fixes#20102.
ACKs for top commit:
MarcoFalke:
re-ACK 90ae3d8ca6📢
Tree-SHA512: 8ffb617053b5f4a8b055da17c06711fd19632e0037d71c4c8135e50c8cd7a19163989484e4e0f17a6cc48bd597f04ecbfd609aef54b7d1d1e76a784214fcf72a
Add early exit guard clauses if node.RelayAddrsWithConn() is false or if
current_time < node.m_next_addr_send. Add comments.
This commit leaves some lines over-indented. Those will be fixed in a
subsequent whitespace-only commit.
beead33a21 [test] no send feefilters when txrelay is turned off (glozow)
18a9b27dd6 p2p: Don't send FEEFILTER in blocksonly mode (Martin Zumsande)
Pull request description:
The purpose of FEEFILTER messages (BIP 133) is to inform our peers that we do not want transactions below a specified fee rate.
In blocksonly mode, we do not want our peer to send us any transactions at all (and will disconnect if a peer still sends a transaction INV or TX).
Therefore, I don't think that it makes sense to send FEEFILTER messages every 10 minutes on average in blocksonly mode - this PR disables it.
Note that on block-relay-only connections, FEEFILTER is already disabled, just not in blocksonly mode.
ACKs for top commit:
glozow:
re ACK beead33a21🙂 thanks for adding the test!
amitiuttarwar:
reACK beead33a21
MarcoFalke:
review ACK beead33a21
jnewbery:
reACK beead33a21
Tree-SHA512: e748cd52fe23d647fa49008b020389956ac508e16ce9fd108d8afb773bff95788298ae229162bd70215d7246fc25c796484966dc05890b0b4ef601f9cd35628b
9ac86bcc0d test: remove qt byteswap compattests (fanquake)
Pull request description:
These were added as part of #9366 when with fixing issues with Protobuf.
Now that we no-longer use Protobuf, there's no reason to maintain a duplicate set of byteswap tests in the qt tests. Our other set of byteswap tests are here: https://github.com/bitcoin/bitcoin/blob/master/src/test/bswap_tests.cpp.
ACKs for top commit:
laanwj:
Code review ACK 9ac86bcc0d
Tree-SHA512: 72ba131a5f8fbd9fdbbc4e1f95baa794496c960b12e0271700c632c6511b7e1b331e8db07a201838b4d56b2aeeb43d4de4e10265ea07ab14241307fa14d3342e
dc4551c22c remove incompatibility release note for darkmode on macos (Sylvain Goumy)
303cfc6227 allow darkmode on macos build (Sylvain Goumy)
78f75a2d60 Allow icon colorization on mac os to better support dark mode (Uplab)
Pull request description:
Allow icons to be colorized on macOS to support native Dark mode color scheme.
Rendering on macOS Big Sur before PR:
Rendering on macOS Big Sur after PR:
Light mode stay visually unchanged.
<del>Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see https://github.com/bitcoin/bitcoin/pull/14593). </del>
<del>But once all glitches are fixed, we will be able to remove this temporary fix. </del>
Edit: this PR is know including the removal of `NSRequiresAquaSystemAppearance` on Info.plist file so that the color fix is apply to every build.
Linked issues: #68#136
ACKs for top commit:
hebasto:
re-ACK dc4551c22c
jarolrod:
ACK dc4551c22c
Tree-SHA512: 1c3a4dec796063e61fcaf80112afc2b15c8669a1cd30ebd537cea96647c20215f8f80289719f905820bb0c490c8c1f94bfae4bb32f9c6d1fdd4e8f199ebb559f
732c7bddeb tests: Add test for CNetAddr::ToString IPv6 address formatting (RFC 5952) (practicalswift)
Pull request description:
Test that `CNetAddr::ToString` formats IPv6 addresses with zero compression and canonicalisation as described in [RFC 5952 ("A Recommendation for IPv6 Address Text Representation")](https://tools.ietf.org/html/rfc5952).
Solving #21466 will hopefully be trivial with the ability to check zero compression correctness against these tests.
ACKs for top commit:
vasild:
ACK 732c7bddeb
Tree-SHA512: 31a1378aa435ba4171490a2e15d7280a175292270eb001b47d367e010c6ac9b83420b82bbeab22211f8f500c69e21878047c87adf216263b3420b6bb2a5d2bfb
These were added as part of #9366 to fix issues with Protobuf.
Now that we no-longer use Protobuf, there's no reason to maintain a
duplicate set of byteswap tests for qt.
257f55c119 qt, refactor: Drop redundant setEditTriggers(NoEditTriggers) calls (Hennadii Stepanov)
Pull request description:
The models of the both views have no `Qt::ItemIsEditable` flag:
3c87dbe95c/src/qt/peertablemodel.cpp (L218-L224)3c87dbe95c/src/qt/bantablemodel.cpp (L148-L154)
ACKs for top commit:
Talkless:
utACK 257f55c119, seems reasonable.
jarolrod:
ACK 257f55c119, looks correct.
Tree-SHA512: 4356e4d785055935fba452488a5d97ed95995def97b26ab18af43a545835f9e9d4c347e4cad7952aa725179cf6e775a2208c48730feebf40e3b1a7ba5f402af0
915e34112b qt: fix issue when disabling the auto-enabled blank wallet checkbox (Jarol Rodriguez)
Pull request description:
As detailed by #151, On `master` a user can create the confusing scenario where you have a disabled `Encrypt Wallet` checkbox and a selected `Disable Private Keys` checkbox after unselecting the auto-enabled `Blank Wallet` checkbox.
This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects `Disable Private keys`, unselecting it will also unselect the `Disable Private Keys` checkbox, which in turn re-enables the `Encrypt Wallet` checkbox.
Below are screenshots comparing the behavior of selecting `Disable Private Keys` then unselecting the `Blank Wallet` between `master` and this `PR`:
**Master:**
| Select `Disable Private Keys` | Unselect `Blank Wallet` |
| ----------------------------- | ------------------------ |
|  |  |
**PR:**
| Select `Disable Private Keys` | Unselect `Blank Wallet` |
| ----------------------------- | ------------------------ |
|  |  |
ACKs for top commit:
hebasto:
ACK 915e34112b
Talkless:
ACK 915e34112b
Tree-SHA512: ce6ecbc35b94a08cabf0b8a24dbdfc874d82cc8918cc8623dce8172c7fc9c75d63a13b036bae5f7ab2c090f8d020574a542285d1651600813faf5d91e2506a8d
4f2653a890 test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c5175c test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1d7b rpc: Improve gettxoutsetinfo help (Fabian Jahr)
Pull request description:
Follow-ups to #19145:
- Small improvement on the help text of RPC gettxoutsetinfo
- Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
- Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`
Split out of #19521.
ACKs for top commit:
MarcoFalke:
review ACK 4f2653a890👲
Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
fa818ca202 fuzz: [refactor] Use PickValue where possible (MarcoFalke)
Pull request description:
`PickValue` is a bit less typing, so I think it should be used where possible
ACKs for top commit:
practicalswift:
cr ACK fa818ca202: patch looks correct and `PickValue` is better :)
Tree-SHA512: 49ed030694e3b7676654f1615f033287d26e2f0bc29647e1db56e0d84e14d29080f3e1898f5df8d644d834b8ded3ce713d2425ea86a37c9279d01f86ad03c202
246774e264 depends: fix Qt precompiled headers bug (Igor Cota)
8e7ad4146d depends: disable Qt Vulkan support on Android (Igor Cota)
ba46adaa1a CI: add Android APK build to cirrus (Igor Cota)
7563720e30 CI: add Android APK build script (Igor Cota)
ebfb10cb75 Qt: add Android packaging support (Igor Cota)
Pull request description:
This PR is the third and final piece of the basic Android support puzzle - it depends on https://github.com/bitcoin/bitcoin/pull/16110 and is related to https://github.com/bitcoin/bitcoin/pull/16883. It introduces an `android` directory under `qt` and a simple way to build an Android package of `bitcoin-qt`:
1. Build depends for Android as described in the [README](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md)
2. Configure with one of the resulting prefixes
3. Run `make && make apk` in `src/qt`
The resulting APK files will be in `android/build/outputs/apk`. You can install them manually or with [adb](https://developer.android.com/studio/command-line/adb). One can also open the `android` directory in Android Studio for that integrated development and debugging experience. `BitcoinQtActivity` is your starting point.
Under the hood makefile `apk` target:
1. Renames the `bitcoin-qt` binary to `libbitcoin-qt.so` and copies it over to a folder under `android/libs` depending on which prefix and corresponding [ABI](https://developer.android.com/ndk/guides/abis.html#sa) `bitcoin-qt` was built for
2. Takes `libc++_shared.so` from the Android NDK and puts in the same place. It [must be included](https://developer.android.com/ndk/guides/cpp-support) in the APK
3. Extracts Qt for Android Java support files from the `qtbase` archive in `depends/sources` to `android/src`
There is also just a tiny bit of `ifdef`'d code to make the Qt Widgets menus usable. It's not pretty but it works and is a stepping stone towards https://github.com/bitcoin/bitcoin/pull/16883.
ACKs for top commit:
MarcoFalke:
cr ACK 246774e264
laanwj:
Code review ACK 246774e264
Tree-SHA512: ba30a746576a167545223c35a51ae60bb0838818779fc152c210f5af1413961b2a6ab6af520ff92cbc8dcd5dcb663e81ca960f021218430c1f76397ed4cead6c
804ac10631 remove unnecessary newline from initWarning() argument (Larry Ruane)
Pull request description:
Run: `src/bitcoind -wallet=nosuchfile`
Without this patch, `debug.log` contains:
```
2021-03-23T21:19:16Z init message: Verifying wallet(s)...
2021-03-23T21:19:16Z Warning: Skipping -wallet path that doesn't exist. Failed to load database path '/home/larry/.bitcoin/wallets/nosuchfile'. Path does not exist.
2021-03-23T21:19:16Z init message: Loading banlist...
```
With this patch, the empty line isn't present. This PR fixes a similar problem with `src/bitcoind -conf=nosuchfile`
ACKs for top commit:
practicalswift:
cr ACK 804ac10631: patch looks correct!
jarolrod:
tACK 804ac10631, nice catch!
theStack:
Code-review ACK 804ac10631
Tree-SHA512: dfcbaaa72ca24ac40233ac56840cfba8827853711d3df6e229ce940686f2ebf8bf0560bafcaa73a4d82d179a5050af0d3cabdc47b3b1dfd6aaadf718a6635f11
This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects 'Disable Private' keys, unselecting it will also unselect the 'Disable Private Keys' checkbox, which in turn re-enables the 'Encrypt Wallet' checkbox.
fac921f23f fuzz: Fix tx_pool target to properly fuzz immature outpoints (MarcoFalke)
fa2b95f861 fuzz: Style fixups (MarcoFalke)
Pull request description:
Also includes a commit for minor style fixups
ACKs for top commit:
glozow:
utACK fac921f23f this fixes it 👍
Tree-SHA512: 1575ba115b2009b653921511c163bd846cd381d6fc92b04a899c0686d23a02bdcdd95c81776b515b80ae187bcec3ccaca3aa88fcecbec888f73ca2d875eef506
d09ebc4723 Fix wrong(1024) divisor for 1000-based prefixes (wodry)
Pull request description:
v.0.21.0
I saw in the GUI peer window in the "received" column `1007 KB`, and after increasing to >=1024 I guess, it switched to `1 MB`. I would have expected the display unit to change from KB to MB already at value >=1000.
I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct.
But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.
To be correct, this PR changes ~~KB/MB/GB to KiB/MiB/GiB.~~ KB to kB and the divisor from 1024 to 1000.
ACKs for top commit:
hebasto:
ACK d09ebc4723, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".
jarolrod:
ACK d09ebc4723
leonardojobim:
Tested ACK d09ebc4723 on Ubuntu 20.04 Qt 5.12.8
Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
1404c57403 [doc] Coin: explain that IsSpent() can also mean never existed (Sjors Provoost)
Pull request description:
This can be especially confusing where `AccessCoin()` is used with logic like this:
```c++
while (iter.n < MAX_OUTPUTS_PER_BLOCK) {
const Coin& alternate = view.AccessCoin(iter);
if (!alternate.IsSpent()) return alternate;
```
ACKs for top commit:
practicalswift:
ACK 1404c57403
MarcoFalke:
ACK 1404c57403
jnewbery:
utACK 1404c57403
Tree-SHA512: 418618dd7e08bd5cc8360e3501d0f57e34100e5101ad3b8e0a819923fa860f44c7f2fada0f8447a1af3c2601fd72bfe619b91ff2f26f7133ceaeb0c98b017b12
55554463c1 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke)
Pull request description:
This has minimally better performance. Reported by me in https://github.com/bitcoin/bitcoin/pull/20228#discussion_r598081787
ACKs for top commit:
practicalswift:
Tested ACK 55554463c1
vasild:
ACK 55554463c1
Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261
fa4cebadcf util: Make Assume() usable as unary expression (MarcoFalke)
Pull request description:
Assume shouldn't behave different at the call site depending on build flags. Currently compilation fails if it is used as expression. Fix that by using the lambda approach from `Assert()` without the `assert()`.
ACKs for top commit:
jnewbery:
ACK fa4cebadcf
practicalswift:
cr ACK fa4cebadcf: patch looks correct and commit hash starts with `fa`
Tree-SHA512: 9ec9ac8d410cdaf5e4e28df571a89e3d23d38e05a7027bb726cae3da6e9314734277e5a218e9e090cc17e10db763da71052c229ad642077ca5824ee42022f3ed
5294f0d5a9 refactor: return std::nullopt instead of {} (fanquake)
Pull request description:
In #21415 [we decided](https://github.com/bitcoin/bitcoin/pull/21415#issuecomment-800236640) to return `std::optional` rather than `{}` for
uninitialized values. This PR replaces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
ACKs for top commit:
hebasto:
ACK 5294f0d5a9, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
Introduce an android directory under qt and allow one to package bitcoin-qt for Android by running make apk.
Add bitcoin-qt Android build instructions.
3d086f42ab test: add ParseUInt16() test coverage (Jon Atack)
Pull request description:
`ParseUInt16()` was just added in #21328.
ACKs for top commit:
practicalswift:
cr ACK 3d086f42ab: patch looks correct & more coverage is better than less coverage
Tree-SHA512: bf7f96deb7c1531419565907f0ea8a8e32b368d4b823a3e80928b2c118edbf643ea06e357b4b5504a89f855caeed289daa9f823c740231ed6ad1b8ed00285ce8
52a43b0c7d build: Fix fuzz-cuckoocache cross-compiling for Windows with DEBUG=1 (Hennadii Stepanov)
Pull request description:
Fix#21348.
ACKs for top commit:
practicalswift:
Tested ACK 52a43b0c7d
Tree-SHA512: 6592f829edfb740a1e9d0691acf04b2372e91b0a53ca395b08350cb0b80031d3b55fa7331bdaddf857d450eb30b605af9fe8fe02559cda19374a48f9634fae70
787df19b09 validation: don't try to invalidate genesis block (Sebastian Falbesoner)
Pull request description:
In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.
Fixes#20914.
ACKs for top commit:
sipa:
ACK 787df19b09. Tested invalidation of generic on regtest.
practicalswift:
Tested ACK 787df19b09
Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
8dd5946c0b add functional test (Larry Ruane)
b5a80fa7e4 util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)
Pull request description:
If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.
ACKs for top commit:
fjahr:
tACK 8dd5946c0b
luke-jr:
utACK 8dd5946c0b (no changes since previous utACK)
hebasto:
re-ACK 8dd5946c0b, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/18335#pullrequestreview-460621350) review.
darosior:
ACK 8dd5946c0b
Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
52dd40a9fe test: add missing netaddress include headers (Jon Atack)
6f09c0f6b5 util: add missing braces and apply clang format to SplitHostPort() (Jon Atack)
2875a764f7 util: add ParseUInt16(), use it in SplitHostPort() (Jon Atack)
6423c8175f p2p, refactor: pass and use uint16_t CService::port as uint16_t (Jon Atack)
Pull request description:
As noticed during review today in https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584873708 of the upcoming I2P network support, `CService::port` is `uint16_t` but is passed around the codebase and into the ctors as `int`, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
ACKs for top commit:
practicalswift:
cr ACK 52dd40a9fe: patch looks correct
MarcoFalke:
cr ACK 52dd40a9fe
vasild:
ACK 52dd40a9fe
Tree-SHA512: 203c1cab3189a206c55ecada77b9548b810281cdc533252b8e3330ae0606b467731c75f730ce9deb07cbaab66facf97e1ffd2051084ff9077cba6750366b0432
Now that we have a reliable way to detect inbound onion peers, this commit
updates our existing eviction protection of 1/4 localhost peers to instead
protect up to 1/4 onion peers (connected via our tor control service), sorted by
longest uptime. Any remaining slots of the 1/4 are then allocated to protect
localhost peers, or 2 localhost peers if no slots remain and 2 or more onion
peers are protected, sorted by longest uptime.
The goal is to avoid penalizing onion peers, due to their higher min ping times
relative to IPv4 and IPv6 peers, and improve our diversity of peer connections.
Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille
for valuable review feedback that shaped the direction.
and an `m_is_onion` struct member to NodeEvictionCandidate and tests.
We'll use these in the peer eviction logic to protect inbound onion peers
in addition to the existing protection of localhost peers.
An unordered set can tell if an element is present in ~O(1) time (constant on
average, worst case linear to the size of the container), which speeds up and
simplifies the lookup in IsEvicted().
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
out of net_tests, because the eviction tests:
- are a different domain of test coverage, with different dependencies
- run more slowly than the net tests
- will be growing in size, in this PR branch and in the future, as eviction
test coverage is improved
to allow deterministic unit testing of the ratio-based peer eviction protection
logic, which protects peers having longer connection times and those connected
via higher-latency networks.
Add documentation.
fa81773243 style-only: Remove whitespace (MarcoFalke)
fae77b9e6d net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)
Pull request description:
* Clarify that "ignoring" really means "disconnect" in the log
* Revive a refactor I took from #13670
ACKs for top commit:
jnewbery:
utACK fa81773243
sipa:
utACK fa81773243
Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
fa2a80bf12 refactor: Pass PeerManagerImpl members only once (MarcoFalke)
Pull request description:
Member variables are already passed to methods via `this`, so no need to pass them another time as function parameter.
ACKs for top commit:
jnewbery:
utACK fa2a80bf12
amitiuttarwar:
utACK fa2a80bf12
Tree-SHA512: 1743825c7560cc748235e3db03e4cea02ad1f670f1b898d7757da644f12693ba9bb2d3eb09b64b3d10dd2e68f52dea31e26d5e97bdc013759baa0515d3c7055c
680eb56d82 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f03 [net processing] Move RelayTransaction into PeerManager (John Newbery)
Pull request description:
This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
ACKs for top commit:
ajtowns:
ACK 680eb56d82
Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
57ff5a42ab doc: specify minimum HWI version (Sjors Provoost)
03308b2bfa rpc: don't require wallet for enumeratesigners (Sjors Provoost)
Pull request description:
HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0
As of #16546 we already rely on features that are in 2.0 and not in the previous 1.* releases:
* `--chain` param
This shouldn't be a problem, because HWI 2.0 has been released before we release v22.
Misc improvements:
* document that HWI 2.0 is required
* drop wallet requirement for `enumeratesigners`
ACKs for top commit:
achow101:
Code Review ACK 57ff5a42ab
Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.
Does not change behavior.
Make sure that all fee calculations use the same feerate.
coin_selection_params.effective_fee is the variable we use for all fee
calculations, so get rid of remaining nFeeRateNeeded usages and just
directly set coin_selection_params.effective_fee.
Does not change behavior.
During each loop of CreateTransaction, instead of constantly getting a
new feerate, use the feerate that we have already fetched for all
fee calculations. Thix fixes a race condition where the feerate required
changes during each iteration of the loop.
This commit changes behavior as the "Fee estimation failed" error will
now take priority over "Signing transaction failed".
4d008f908e Always add -daemonwait to known command line arguments (Hennadii Stepanov)
Pull request description:
This is a follow up of #21007.
When `AC_CHECK_DECLS([fork])` fails:
- on master (8e6532053f):
```
$ src/bitcoind -daemonwait
Error: Error parsing command line arguments: Invalid parameter -daemonwait
```
- with this PR:
```
$ src/bitcoind -daemonwait
Error: -daemon is not supported on this operating system
```
ACKs for top commit:
laanwj:
Code review ACK 4d008f908e
Tree-SHA512: 7fcb5e9d76958adcf57e04fa74bd2a98d62459d81a3c57a97bd74c346cbf47c53e560a15455fb024e912c3b44e8487a83499e993b282871ba069953e665d88a9
Add a regression test for https://github.com/bitcoin/bitcoin/pull/21407.
The test creates a socket that, upon read, returns some data, but never
the expected terminator `\n`, injects that socket into the I2P code and
expects `i2p::sam::Session::Connect()` to fail, printing a specific
error message to the log.
Change the types of `i2p::Connection::sock` and
`i2p::sam::Session::m_control_sock` from `Sock` to
`std::unique_ptr<Sock>`.
Using pointers would allow us to sneak `FuzzedSock` instead of `Sock`
and have the methods of the former called.
After this change a test only needs to replace `CreateSock()` with
a function that returns `FuzzedSock`.
Change `ConnectSocketDirectly()` to take a `Sock` argument instead of a
bare `SOCKET`. With this, use the `Sock`'s (possibly mocked) methods
`Connect()`, `Wait()` and `GetSockOpt()` instead of calling the OS
functions directly.
If `recv(2)` returns an error (`-1`) and sets `errno` to a temporary
error like `EAGAIN` a proper application code is expected to retry the
operation.
If the fuzz data is exhausted, then `FuzzedSock::Recv()` will keep
returning `-1` and setting `errno` to the first element of
`recv_errnos[]` which happened to be `EAGAIN`. This may continue forever
or cause the fuzz test to run for a long time before some higher level
application "receive timeout" is triggered.
Thus, put `ECONNREFUSED` as first element of `recv_errnos[]`.
A conforming `recv(2)` call is supposed to return the same data on a
call following `recv(..., MSG_PEEK)`. Extend `FuzzedSock::Recv()` to do
that.
For simplicity we only return 1 byte when `MSG_PEEK` is used. If we
would return a buffer of N bytes, then we would have to keep track how
many of them were consumed on subsequent non-`MSG_PEEK` calls.
We want `Get()` to always return the same value, otherwise it will look
like the `FuzzedSock` implementation itself is broken. So assign
`m_socket` a random number in the `FuzzedSock` constructor.
There is nothing to fuzz about the `Get()` and `Release()` methods, so
use the ones from the base class `Sock`.
`Reset()` is just setting our socket to `INVALID_SOCKET`. We don't want
to use the base `Reset()` because it will close `m_socket` and given
that our `m_socket` is just a random number it may end up closing a real
opened file descriptor if it coincides with our random `m_socket`.
7059e6d822 test: add a test to ensure RecvUntilTerminator() limit works (Vasil Dimov)
80a5a8ea2b i2p: limit the size of incoming messages (Vasil Dimov)
Pull request description:
Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.
In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
ACKs for top commit:
laanwj:
Re-ACK 7059e6d822
Tree-SHA512: 21f3f3468c765c726cdc877eae847cdb4dbe225d94c5bd1849bd752c9761fac881c554f16ea7a685ad40312159d554e126c481e21c5fd83a6d947060b920373d
88df300f20 qt: Do not translate file extensions (Hennadii Stepanov)
Pull request description:
File extensions are untranslatable by their nature.
ACKs for top commit:
laanwj:
Concept and code review ACK 88df300f20
Talkless:
tACK 88df300f20, tested on Debian Sid with Qt 5.15.2. Tested all filters except for .psbt.
jarolrod:
re-ACK 88df300f20
Tree-SHA512: 104d383543edcee8fb825f98d3b6669a7aaae2c74b6602f9bc407bf1c88be121ec535f2f9be87afa6ca775dc79865165f620553f6f6ab1d31a3f9ea93f7f9593
76782e560b refactor: reuse test string with embedded null char in util_tests (Jon Atack)
24c6546946 test: add ParseUInt8() unit and fuzz test coverage (Jon Atack)
Pull request description:
We have unit test and fuzzer coverage for
- `ParseInt64()`
- `ParseInt32()`
- `ParseUInt64()`
- `ParseUInt32()`
but not `ParseUInt8()`, so this pull adds it.
I was tempted to add a commit that applies clang formatting to the file, or one that updates the C-style casts to named casts, but resisted the temptation unless reviewers request it.
ACKs for top commit:
laanwj:
Code review ACK 76782e560b
MarcoFalke:
cr ACK 76782e560b
Tree-SHA512: 1d7948b3385632094a3b0f0e38f87dccddabf74002e68aa055a51408866b057828ffa15c4b22aa9adde458155fbb5e443b66a9dbf3d7713358fc98a14d64bdcf
52f0be3a93 compat: remove memcpy -> memmove backwards compatibility alias (fanquake)
Pull request description:
In glib 2.13 memcpy was changed such that the way it copied bytes was reversed.
This caused all sorts of issues for existing software, which depended on the
existing behavior (when they should have been using memmove). See:
https://sourceware.org/bugzilla/show_bug.cgi?id=12518https://bugzilla.redhat.com/show_bug.cgi?id=638477
Now that we require glibc 2.17+ (#17538), we should be well clear of having to
maintain our memcpy -> memmove aliasing, which was introduced in #4339.
Gitian builds:
```bash
# Linux:
52dee59c8c7d5620ac9b140b79fcaf3d2f15a219293140190f9283ba871f5391 bitcoin-52f0be3a9332-aarch64-linux-gnu-debug.tar.gz
8963473b8791c5c6033a992d7dd761832fe1fb5732be790a6e9f8c11d67ad8ae bitcoin-52f0be3a9332-aarch64-linux-gnu.tar.gz
1fb3365c1ef60ecd1eb2d18f671f8f1e8cde0585de7de74aa0c5121093100c26 bitcoin-52f0be3a9332-arm-linux-gnueabihf-debug.tar.gz
305c5b032d51ba97459715211112204a09d119edd6ec2a12b796559ad3fde761 bitcoin-52f0be3a9332-arm-linux-gnueabihf.tar.gz
1f950a3e3979a4e1a67696b3fddc3090a0489a43b49e2b58a348d4b02ada2aa8 bitcoin-52f0be3a9332-powerpc64-linux-gnu-debug.tar.gz
0b9731dba768b30c91dadec4cd7a98c86e06fbf6354555f798b46b7c4fab7b5f bitcoin-52f0be3a9332-powerpc64-linux-gnu.tar.gz
c4a37aae56cc023964f8d9e82d1b66913079cab559cbfc1c9127969aa968a06f bitcoin-52f0be3a9332-powerpc64le-linux-gnu-debug.tar.gz
dfbaa4f3bf12988a0a7f82c4b10162e5e7a63382a7e29d0170bc32ce344c97c3 bitcoin-52f0be3a9332-powerpc64le-linux-gnu.tar.gz
3a0280d2c06516e50b0841d6f42d9589355dc9a1f8bb9a0b123554cd91b08004 bitcoin-52f0be3a9332-riscv64-linux-gnu-debug.tar.gz
cc199a0f254b2366e80a6a884120ec3ea442983990ba1a5eb993c36060686eba bitcoin-52f0be3a9332-riscv64-linux-gnu.tar.gz
eb8e7ca673cc06c167ab082fe457a41f73758ecd5b34941300e3cd378c29b197 bitcoin-52f0be3a9332-x86_64-linux-gnu-debug.tar.gz
dad19226c0e4c54b78ca2fa85fc28c5bfd1e1178e3f765472bd2f895a1d57145 bitcoin-52f0be3a9332-x86_64-linux-gnu.tar.gz
ef89be95b84bb7c6fef055634cd20caf2fa5b42441502918dbfbf758bb2daab6 src/bitcoin-52f0be3a9332.tar.gz
dc61f5ca33330c1609bc56b23f39fef3c1ff5ec6a1799d5b7a18f3c3b3acc9f9 bitcoin-core-linux-22-res.yml
```
Guix builds:
```bash
b50d6399cb59e5e4a9247b12a3eda61de6e51bd87ef1f27b388b75b71dfccf92 output/bitcoin-52f0be3a9332-aarch64-linux-gnu-debug.tar.gz
23d845dc13e60a581ebdfbaa6063f559a56cce06734e1b50790d2fc13e257793 output/bitcoin-52f0be3a9332-aarch64-linux-gnu.tar.gz
79094406fe00939bbce17a6d65de5a2686625e871432350c69e674cc80b1491c output/bitcoin-52f0be3a9332-arm-linux-gnueabihf-debug.tar.gz
65a91913249a743015eceea5a56c497d606af17270cb7e8a3df10cf729b757ec output/bitcoin-52f0be3a9332-arm-linux-gnueabihf.tar.gz
5e75ca5e8cf6934ba5a5a1b4d26c1b361b118e10ef34b73845d038035ddb9b85 output/bitcoin-52f0be3a9332-osx-unsigned.dmg
774b372696cde8ceab40f6909dadea3fc87b375b495fcfb4ee8a963afd7fbd3a output/bitcoin-52f0be3a9332-osx-unsigned.tar.gz
dc4bdfb7b32dcc0b6e876d6d7ab3cb8d1472f21f66546ab70515f96262292e21 output/bitcoin-52f0be3a9332-osx64.tar.gz
ea178ff9e28439f80129445cf260215c74eea2e610f62ff045061f287675d3ff output/bitcoin-52f0be3a9332-powerpc64-linux-gnu-debug.tar.gz
0390687a7aaa3f0a8a78be2deab21116599e5b332f00a2d1fdce97a5bd30e3eb output/bitcoin-52f0be3a9332-powerpc64-linux-gnu.tar.gz
52c948719a27f252f5969558abc2718c1e365ea85496322cb4ec97eab8a234cc output/bitcoin-52f0be3a9332-powerpc64le-linux-gnu-debug.tar.gz
5a4a8748dffe7e6a5bd07f3f564b1f2052440c4199fe25aaa41675bfb69e61db output/bitcoin-52f0be3a9332-powerpc64le-linux-gnu.tar.gz
ba521bd2b4e73aea317821a9e08da9a326c0be3b38d923b35ba14bc68ee6c814 output/bitcoin-52f0be3a9332-riscv64-linux-gnu-debug.tar.gz
783ea81ab2f6b642b13ebf7882aa822d12f95936574a8848a74b1b8978e6801d output/bitcoin-52f0be3a9332-riscv64-linux-gnu.tar.gz
376706fc12e58d7d559a87e1ce64be22eaac3fc32d95c60d603ad893d9128cc1 output/bitcoin-52f0be3a9332-win-unsigned.tar.gz
7aa48242fb71e29b00992b2be8677f1ea49f2ca82c5355bf0c1d4c8d14635596 output/bitcoin-52f0be3a9332-win64-debug.zip
41e6461ab573fa8f6ac0f198193e72a4a047bb7a4193f743b937e81739c929cc output/bitcoin-52f0be3a9332-win64-setup-unsigned.exe
e2c4ecb05f24577da12f722d848bf6ac89f3f549d6d2bfd30d65676099c0725b output/bitcoin-52f0be3a9332-win64.zip
60ed63b3b562fa2141f18f1556a03c2474b75797088cd68fdb3e7d057a6983a3 output/bitcoin-52f0be3a9332-x86_64-linux-gnu-debug.tar.gz
adb0bb62dc8b99d025a863e921b8e670f4c8f4b5600cd6d79eb552ede10bc8b8 output/bitcoin-52f0be3a9332-x86_64-linux-gnu.tar.gz
ef89be95b84bb7c6fef055634cd20caf2fa5b42441502918dbfbf758bb2daab6 output/src/bitcoin-52f0be3a9332.tar.gz
```
ACKs for top commit:
laanwj:
Concept and code review ACK 52f0be3a93
Tree-SHA512: 851634a633cc7d27b10f11436768f3695a7615d5850166c3718028c36d3a7dd56baa2dd1028f47802891703e9f5a1d382f559e388ecef2249e2004edc62d97bf
8348a3742b net: fix hSocket param in netbase.h::ConnectSocketDirectly() (Jon Atack)
e6bd74b2e5 net: move Doxygen docs from netbase.cpp to netbase.h (Jon Atack)
12cc5704db net: update incorrect Doxygen documentation in netbase.cpp (Jon Atack)
Pull request description:
While doing #21328, I noticed docs that were out-of-date or in the wrong file.
The second commit is essentially move-only and is best reviewed with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
ACKs for top commit:
laanwj:
Code review ACK 8348a3742b
Tree-SHA512: 13dae4abd3009fc43dfffc98e0f7eebcd6ad02afdd6050a7685a2ad4e6aaad93480d93886a2d1bd2375c2439d426494e4a8bc0c60e0e3104bfaa1830831ca663
Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
if no terminator is received.
In the case of I2P this avoids a runaway (or malicious) I2P proxy
sending us tons of data without a terminator before a timeout is
triggered.
77833a364a Revert "qt: Use "fusion" style on macOS Big Sur with old Qt" (Hennadii Stepanov)
Pull request description:
This PR reverts workaround introduced in #177.
After bumping Qt version in depends to 5.12.10 in bitcoin/bitcoin#21376, there are no reasons to use the Fusion style on macOS.
ACKs for top commit:
leonardojobim:
tACK 77833a364a. Tested on macOS Big Sur v11.2.3
jarolrod:
ACK 77833a364a
Talkless:
utACK 77833a364a
Tree-SHA512: f704f2027dd380dfc604231e3606a036a8be891aeeddf643c474131014fa080e123b42836ac643a2064fe7a5a018fa8b9aa61a31f9da1d15880de6a36c4c0d54
This is out of an abundance of caution only, as signet currently doesn't
enable taproot validation flags. Still, it seems cleaner to make sure
that all non-test code that passes MissingDataBehavior::ASSERT_FAIL
also actually makes sure no data can be missing.
Historically lack of amount data has been treated as amount==-1. Change
this and treat it as missing data, as introduced in the previous commits.
To be minimally invasive, do this at SignatureHash() call sites rather
than inside SignatureHash() (which currently has no means or returning
a failure code).
Remove the implicit MissingDataBehavior::ASSERT_FAIL in the
*TransationSignatureChecker constructors, and instead specify
it explicit in all call sites:
* Test code uses ASSERT_FAIL
* Validation uses ASSERT_FAIL (through CachingTransactionSignatureChecker)
(including signet)
* libconsensus uses FAIL, matching the existing behavior of the
non-amount API (and the extended required data for taproot validation
is not available yet)
* Signing code uses FAIL
This allows specifying how *TransactionSignatureChecker will behave when
presented with missing transaction data such as amounts spent, BIP341 data,
or spent outputs.
As all call sites still (implicitly) use MissingDataBehavior::ASSERT_FAIL,
this commit introduces no change in behavior.
