25dc87e6f8 libconsensus: deprecate (Cory Fields)
Pull request description:
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib).
Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway.
See for example the discussions:
https://github.com/hebasto/bitcoin/pull/41https://github.com/bitcoin/bitcoin/pull/29123
And here: https://github.com/bitcoin/bitcoin/pull/29180
Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations.
Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
If there are any users currently using libbitcoinconsensus, please chime in with your use-case!
Edit: Corrected final release to be v27.
ACKs for top commit:
TheCharlatan:
ACK 25dc87e6f8
fanquake:
ACK 25dc87e6f8 - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.
Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638
fad74bbbd0 refactor: Mark prevector iterator with std::contiguous_iterator_tag (MarcoFalke)
fab8a01048 refactor: Fix binary operator+ for prevector iterators (MarcoFalke)
fa44a60b2b refactor: Fix constness for prevector iterators (MarcoFalke)
facaa66b49 refactor: Add missing default constructor to prevector iterators (MarcoFalke)
Pull request description:
Currently prevector iterators have many issues:
* Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like `std::minmax_element`.
* Various `const` issues with random access iterators. For example, a `const iterator` is different from a `const_iterator`, because the first one holds a mutable reference and must also return it without `const`. Also, `operator+` must be callable regardless of the iterator object's `const`-ness.
* When adding an offset to random access iterators, both `x+n` and `n+x` must be specified, see https://eel.is/c++draft/random.access.iterators#tab:randomaccessiterator
Fix all issues.
Also, upgrade the `std::random_access_iterator_tag` (C++17) to `std::contiguous_iterator_tag` (C++20)
ACKs for top commit:
TheCharlatan:
ACK fad74bbbd0
stickies-v:
ACK fad74bbbd0
willcl-ark:
ACK fad74bbbd0
Tree-SHA512: b1ca778a31602af94b323b8feaf993833ec78be09f1d438a68335485a4ba97f52125fdd977ffb9541b89f8d45be0105076aa07b5726936133519aae832556e0b
b851c5385d fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses (Vasil Dimov)
Pull request description:
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
ACKs for top commit:
achow101:
ACK b851c5385d
mzumsande:
ACK b851c5385d
brunoerg:
utACK b851c5385d
Tree-SHA512: 9905acff0e996f30ddac0c14e5ee9e1db926c7751472c06d6441111304242b563f7c942b162b209d80e8fb65a97249792eef9ae0a96100419565bf7f59f59676
9642aefb81 test: fix intermittent failure in p2p_v2_earlykeyresponse (Martin Zumsande)
Pull request description:
The test fails intermittently, see https://cirrus-ci.com/task/6403578080788480?logs=ci#L3521 and https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1916996716.
I think it's because of a race between the python NetworkThread and the actual
test, which will both call `initiate_v2_handshake`. I could reproduce it by adding a sleep into `initiate_v2_handshake` after the line `self.sent_garbage = random.randbytes(garbage_len)`.
Fix this by waiting for the first `initiate_v2_handshake` to have finished before calling it a second time.
ACKs for top commit:
stratospher:
tested ACK 9642aef.
achow101:
ACK 9642aefb81
theStack:
Tested ACK 9642aefb81
Tree-SHA512: f728bbceaf816ddefeee4957494ccb608ad4fc912cb5cbf5f2acf09836df969c4e8fa2bb441aadb94fa39b3ffbb005d4132e7b6a5a98d80811810d8bd1d624e3
987a1b51ee init: settings, do not load auto-generated warning msg (furszy)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1907071391.
The settings warning message is meant to be used only to discourage users from
modifying the file manually. Therefore, there is no need to keep it in memory.
ACKs for top commit:
achow101:
ACK 987a1b51ee
ryanofsky:
Code review ACK 987a1b51ee. Seems like a clean, simple fix
Tree-SHA512: 3f2bdcf4b4a9cadb396dcff9b43155211eeed018527a07356970a341d139ad18edbd1a4d369377c8907b8ec1f19ee2ab8aacf85a887379e6d57a8a6db2403d51
c11c404281 tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d4 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1ed wallet: Skip key and script migration for blank wallets (Andrew Chow)
Pull request description:
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110
ACKs for top commit:
furszy:
reACK c11c404281. CI failure is unrelated.
ryanofsky:
Code review ACK c11c404281
Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
ff9039f6ea Remove GetAdjustedTime (dergoegge)
Pull request description:
This picks up parts of #25908.
The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains.
ACKs for top commit:
naumenkogs:
ACK ff9039f6ea
achow101:
ACK ff9039f6ea
maflcko:
lgtm ACK ff9039f6ea🤽
stickies-v:
ACK ff9039f6ea
Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
0bef1042ce net: enable v2transport by default (Pieter Wuille)
Pull request description:
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.
ACKs for top commit:
stratospher:
ACK 0bef104.
josibake:
reACK 0bef1042ce
achow101:
ACK 0bef1042ce
naumenkogs:
ACK 0bef1042ce
theStack:
ACK 0bef1042ce
willcl-ark:
crACK 0bef1042ce
BrandonOdiwuor:
utACK 0bef1042ce
pablomartin4btc:
re ACK 0bef1042ce
kristapsk:
utACK 0bef1042ce
Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
b298242c8d test: sqlite, add coverage for dangling to-be-reverted db txns (furszy)
fc0e747192 sqlite: guard against dangling to-be-reverted db transactions (furszy)
472d2ca981 sqlite: introduce HasActiveTxn method (furszy)
dca874e838 sqlite: add ability to interrupt statements (furszy)
fdf9f66909 test: wallet db, exercise deadlock after write failure (furszy)
Pull request description:
Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931.
If the db handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reverted; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the isolation property but also results in the
improper storage of incomplete information on disk, impacting
the wallet consistency.
This PR fixes the issue by resetting the db connection, automatically
rolling back the transaction (per https://www.sqlite.org/c3ref/close.html)
when the handler object is being destroyed and the txn abortion failed.
Testing Notes
Can verify the failure by reverting the fix e5217fea and running the test.
It will fail without e5217fea and pass with it.
ACKs for top commit:
achow101:
ACK b298242c8d
ryanofsky:
Code review ACK b298242c8d. Just fix for exec result codes and comment update since last review.
Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb
27f260aa6e net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92b15 test: add coverage for peerman adaptive connections service flags (furszy)
6ed53602ac net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e591c5 net: move state dependent peer services flags (furszy)
f9ac96b8d6 net: decouple state independent service flags from desirable ones (furszy)
97df4e3887 net: store best block tip time inside PeerManager (furszy)
Pull request description:
Derived from #28120 discussion.
By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.
The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections with limited peers, filling up all available outbound slots. Resulting
in an inability to synchronize the chain (because limited peers will not provide
blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).
ACKs for top commit:
achow101:
ACK 27f260aa6e
vasild:
ACK 27f260aa6e
naumenkogs:
ACK 27f260aa6e
mzumsande:
Light Code Review ACK 27f260aa6e
andrewtoth:
ACK 27f260aa6e
Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
26ad2aeb29 test: fix wallet_import_rescan unrounded minimum amount (stickies-v)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1468842089.
Fixes a `JSONRPCException: Invalid amount (-3)` exception by ensuring the amount sent to `sendtoaddress` is rounded to 8 decimals.
See https://cirrus-ci.com/task/5562947183837184?logs=ci#L2559
Note: since `round` can also round down, `min_amount` is not _exactly_ guaranteed, but this is not a problem for the current usage. I've added a docstring to highlight this.
ACKs for top commit:
sr-gi:
ACK [26ad2ae](26ad2aeb29)
Tree-SHA512: 82ce16447f30535f17fa73336f7e4f74639e33215a228294b9b8005b8050a760b90a3726de279cce98c7e439f09104172b74072be3a300dbd461bf0c3f54b954
This library has existed for nearly 10 years with very little known uptake or
impact. It has become a maintenance burden. In several cases it dictates our
code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as
well as build-system procedures (building multiple copies of object files
especially for the lib).
Several discussions have arisen wrt migrating it to CMake and it has become
difficult to justify adding more complexity for a library that is virtually
unused anyway.
See for example the discussions:
https://github.com/hebasto/bitcoin/pull/41https://github.com/bitcoin/bitcoin/pull/29123
Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not
migrating it to CMake and letting it end with v27. Any remaining use-cases
could be handled in the future by libbitcoinkernel.
If the handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reversed; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the database isolation property but also results
in the improper storage of incomplete information on disk, impacting
the wallet consistency.
By encapsulating sqlite3_exec into its own standalone method
and introducing the 'SQliteExecHandler' class, we enable the
ability to test db statements execution failures within the
unit test framework.
This is used in the following-up commit to exercise a deadlock
and improve our wallet db error handling code.
Moreover, the future encapsulation of other sqlite functions
within this class will contribute to minimize the impact of
any future API changes.
31cce4a1bd doc: update `BroadcastTransaction` comment (ismaelsadeeq)
Pull request description:
`BroadcastTransaction` is also called by `submitpackage` RPC.
All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
ea4ddd8652/src/rpc/mempool.cpp (L926)
It's not maintainable to list all the callers of a function.
ACKs for top commit:
stickies-v:
ACK 31cce4a1bd
kristapsk:
ACK 31cce4a1bd
naumenkogs:
ACK 31cce4a1bd
Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
9819db4cca validation: move nChainTx assert down in CheckBlockIndex (Martin Zumsande)
033477dba6 doc: fix checkblockindex comments (Martin Zumsande)
Pull request description:
The two assumptions there were described as test-only, which has led to confusion whether they should exist.
However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled.
The second commit moves this assert down to the other checks.
Closes#29261
ACKs for top commit:
maflcko:
ACK 9819db4cca 🌦
naumenkogs:
ACK 9819db4cca
ryanofsky:
Code review ACK 9819db4cca. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.
Tree-SHA512: 3f77791253eb0c97f8153dd8ae1c567f43f6387ea7a53efea94817463c672a4e11d548aa7eff62235346ff0713ff4d6fe08f9ec50d0c30a1e6b6d27b9918b419
55556a64a8 test: Remove struct import from messages.py (MarcoFalke)
fa3fa86dda scripted-diff: test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fafc0d68ee test: Use int from_bytes and to_bytes over struct packing (MarcoFalke)
fa3886b7c6 test: Treat msg_version.relay as unsigned (MarcoFalke)
Pull request description:
`struct` has many issues in messages.py:
* For unpacking, it requires to specify the length a second time, even when it is already clear from the `f.read(num_bytes)` context.
* For unpacking, it is designed to support a long format string and returning a tuple of many values. However, except for 3 instances in `messages.py`, usually only a single value is unpacked and all those cases require an `[0]` access.
* For packing and unpacking of a single value, the format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
I presume the above issues lead to accidentally treat `msg_version.relay` as a "signed bool", which is fine, but confusing.
Fix all issues by using the built-in `int` helpers `to_bytes` and `from_bytes` via a scripted diff.
Review notes:
* `struct.unpack` throws an error if the number of bytes passed is incorrect. `int.from_bytes` doesn't know about "missing" bytes and treats an empty byte array as `int(0)`. "Extraneous" bytes should never happen, because all `read` calls are limited in this file. If it is important to keep this error behavior, a helper `int_from_stream(stream, num_bytes, bytes, byteorder, *, **kwargs)` can be added, which checks the number of bytes read from the stream.
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
ACKs for top commit:
stickies-v:
ACK 55556a64a8
theStack:
re-ACK 55556a64a8
Tree-SHA512: 1cef8cdfd763fb424ed4b8be850a834b83fd0ef47fbea626a29784eb4f4832d44e42c4fe05b298b6070a933ef278b0222289a9955a97c86707e091e20bbb247a
bc9283c441 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher)
ffe6a56d75 [test] Check whether v2 TestNode performs downgrading (stratospher)
ba737358a3 [test] Add functional tests to test v2 P2P behaviour (stratospher)
4115cf9956 [test] Ignore BIP324 decoy messages (stratospher)
8c054aa04d [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher)
382894c3ac [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher)
a94e350ac0 [test] Build v2 P2P messages (stratospher)
bb7bffed79 [test] Use lock for sending P2P messages in test framework (stratospher)
5b91fb14ab [test] Read v2 P2P messages (stratospher)
05bddb20f5 [test] Perform initial v2 handshake (stratospher)
a049d1bd08 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher)
b89fa59e71 [test] Construct class to handle v2 P2P protocol functions (stratospher)
8d6c848a48 [test] Move MAGIC_BYTES to messages.py (stratospher)
595ad4b168 [test/crypto] Add ECDH (stratospher)
4487b80517 [rpc/net] Allow v2 p2p support in addconnection (stratospher)
Pull request description:
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
### commits overview
1. introduces a new class `EncryptedP2PState` to store the keys, functions for performing the initial v2 handshake and encryption/decryption.
3. this class is used by `P2PConnection` in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection)
- `v2_state` is the object of class `EncryptedP2PState` in `P2PConnection` used to store its keys, session-id etc.
- a node [advertising](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#advertising-to-support-v2-p2p) support for v2 P2P is different from a node actually [supporting v2 P2P](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#supporting-v2-p2p) (differ when false advertisement of services occur)
- introduce a boolean variable `supports_v2_p2p` in `P2PConnection` to denote if it supports v2 P2P.
- introduce a boolean variable `advertises_v2_p2p` to denote whether `P2PConnection` which mimics peer behaviour advertises V2 P2P support. Default option is `False`.
- In the test framework, you can create Inbound and Outbound connections to `TestNode`
1. During **Inbound Connections**, `P2PConnection` is the initiator [`TestNode` <--------- `P2PConnection`]
- Case 1:
- if the `TestNode` advertises/signals v2 P2P support (means `self.nodes[i]` set up with `"-v2transport=1"`), different behaviour will be exhibited based on whether:
1. `P2PConnection` supports v2 P2P
2. `P2PConnection` does not support v2 P2P
- In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if `P2PConnection` should support v2 P2P or not. So `supports_v2_p2p` boolean variable is used as an option to enable support for v2 P2P in `P2PConnection`.
- Since the `TestNode` advertises v2 P2P support (using "-v2transport=1"), our initiator `P2PConnection` would send:
1. (if the `P2PConnection` supports v2 P2P) ellswift + garbage bytes to initiate the connection
2. (if the `P2PConnection` does not support v2 P2P) version message to initiate the connection
- Case 2:
- if the `TestNode` doesn't signal v2 P2P support; `P2PConnection` being the initiator would send version message to initiate a connection.
2. During **Outbound Connections** [TestNode --------> P2PConnection]
- initiator `TestNode` would send:
- (if the `P2PConnection` advertises v2 P2P) ellswift + garbage bytes to initiate the connection
- (if the `P2PConnection` advertises v2 P2P) version message to initiate the connection
- Suppose `P2PConnection` advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario)
- `TestNode` sends ellswift + garbage bytes
- `P2PConnection` receives but can't process it and disconnects.
- `TestNode` then tries using v1 P2P and sends version message
- `P2PConnection` receives/processes this successfully and they communicate on v1 P2P
4. the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC
5. includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious
intermediary)
### run the tests
* functional test - `test/functional/p2p_v2_encrypted.py` `test/functional/p2p_v2_earlykeyresponse.py`
I'm also super grateful to @ dhruv for his really valuable feedback on this branch.
Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md
ACKs for top commit:
naumenkogs:
ACK bc9283c441
mzumsande:
Code Review ACK bc9283c441
theStack:
Code-review ACK bc9283c441
glozow:
ACK bc9283c441
Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
8531e1e731 ci: Use DEBUG=1 in depends for MSAN jobs (fanquake)
800ddef6b9 ci: use LLVM 17.0.6 in MSAN jobs (fanquake)
Pull request description:
Switch to using LLVM 17.0.6 and `DEBUG=1` in MSAN CI jobs.
ACKs for top commit:
maflcko:
lgtm ACK 8531e1e731
Tree-SHA512: 819889762aeb78f95c4f955978890c6d98884bed0c7ff97ec072f4c7c1119ee3e3268ccab795bb1c801d36a206e16c6c1195e7a2bc7af94b580d17e49c632161
fab97d81ce fuzz: Print coverage summary after run_once (MarcoFalke)
Pull request description:
This can be used to quickly check the coverage effects of a code change or qa-assets change.
ACKs for top commit:
dergoegge:
ACK fab97d81ce
Tree-SHA512: 0ac913c14698f39e76e0e7bf124f182220031796d6443edb34c6e4615e128157cf746da661b216c4640a41964e977249712445ca9c005b1b4a3737adabdb4a7d
5b9d5bf866 depends: remove (darwin) libtool now that it's no longer used (Cory Fields)
3ef6563495 depends: use ar rather than libtool for miniupnpc/libnatpmp (Cory Fields)
Pull request description:
An alternative to https://github.com/bitcoin/bitcoin/pull/29232
Rather than switching to the CMake builds which [proved problematic](https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1898513919), do the quick and dirty thing of just patching out libtool. Doesn't seem to introduce any new issues.
This should buy us time to upstream the necessary CMake fixes.
ACKs for top commit:
TheCharlatan:
ACK 5b9d5bf866
fanquake:
ACK 5b9d5bf866
Tree-SHA512: c75c4bcc9332d8c1fc3395e2b5fc7265849186afc7005700f662ab291e6ea1f111025fad733d0b0b39d35029d1b757d3f1937d63aad3c0c3b88d0f8ac902ee18
The C++ code treats bool as uint8_t, so the python tests should as well.
This also allows to simplify the code, because converting an empty byte
array to int gives int(0).
>>> int.from_bytes(b'')
0
bbf218d061 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields)
4dbd0475d8 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields)
Pull request description:
Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.
Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.
Removing the define should have the effect of enabling sha256 optimizations for the kernel.
ACKs for top commit:
TheCharlatan:
Re-ACK bbf218d061
hebasto:
re-ACK bbf218d061
Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
3bfc5bd36e test: ensure output is large enough to pay for its fees (stickies-v)
Pull request description:
Fixes a (rare) intermittency issue in wallet_import_rescan.py
Since we [use](03752444cd/test/functional/wallet_import_rescan.py (L296)) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.
Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log
```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is rescanned as well if the rescan parameter is set to true
2024-01-18T22:16:20.187000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_import_rescan.py", line 292, in run_test
child = self.nodes[1].send(
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: The transaction amount is too small to pay the fee (-4)
```
Can be reproduced locally by forcing usage of the lowest possible value produced by `get_rand_amount()` ([thanks furszy](https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1832956095)):
<details>
<summary>git diff on 5f3a0574c4</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 7f01d23941..925849d5c0 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -270,7 +270,7 @@ class ImportRescanTest(BitcoinTestFramework):
address_type=variant.address_type.value,
))
variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
- variant.initial_amount = get_rand_amount() * 2
+ variant.initial_amount = Decimal(str(round(AMOUNT_DUST, 8))) * 2
variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
variant.confirmation_height = 0
variant.timestamp = timestamp
```
</details>
ACKs for top commit:
achow101:
ACK 3bfc5bd36e
glozow:
utACK 3bfc5bd36e, didn't experience this issue but in theory a minimum of `AMOUNT_DUST` could be too low to pay the fees
furszy:
utACK 3bfc5bd36
Tree-SHA512: 821ab94a510772e90528b2cef368bbf70309d8fd1dcda53dce75dd1bf91622358e80fea4d9fc68249b9d598892306c66f6c843b4a6855a9f9a9175f7b41109c6
8023640a71 qt: Avoid non-self-contained Windows header (Hennadii Stepanov)
Pull request description:
Using the `windows.h` header guarantees correctness regardless of the content of other headers.
For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio
Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.
Related to https://github.com/hebasto/bitcoin/pull/77.
ACKs for top commit:
theuni:
ACK 8023640a71. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.
Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
cf937b2068 fuzz: also set MSAN_SYMBOLIZER_PATH (fanquake)
Pull request description:
Should resolve: https://github.com/bitcoin-core/qa-assets/issues/167.
ACKs for top commit:
dergoegge:
utACK cf937b2068
Tree-SHA512: a7670b5054c2c9ec830db2a4dd4d78d8a0ee7d793a80d32942d78b5e459015344040fa9ce9d73f4f23cd920d5ca2e65c110e201723e4935de8f57fda0b6d5ce7
fa3373d3ad refactor: Compile unreachable code (MarcoFalke)
Pull request description:
When unreachable code isn't compiled, compile failures are not detected.
Fix this by leaving it unreachable, but compiling it.
Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916
ACKs for top commit:
achow101:
ACK fa3373d3ad
ryanofsky:
Code review ACK fa3373d3ad. This looks good, and should prevent code in the else blocks from accidentally breaking.
Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e
d298ff8b62 During IBD, prune as much as possible until we get close to where we will eventually keep blocks (Luke Dashjr)
Pull request description:
This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache.
Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks.
ACKs for top commit:
andrewtoth:
ACK d298ff8b62
fjahr:
utACK d298ff8b62
achow101:
ACK d298ff8b62
Tree-SHA512: 2a482376bfb177e2ba7c2f0bb0b58b02efdb38b34755a18d1fc3e869df5959c85b6f1009e1386fa8b89c4f90d520383e36bd3e21dec221042315134efb1a455b
5fb8f0f80f depends: Do not override CFLAGS when building SQLite with DEBUG=1 (Hennadii Stepanov)
2b0dd88f1c depends: Ensure definitions are passed when building SQLite with DEBUG=1 (Hennadii Stepanov)
Pull request description:
The `--enable-debug` configure option for the SQLite package does two things:
```autoconf
#-----------------------------------------------------------------------
# --enable-debug
#
AC_ARG_ENABLE(debug, [AS_HELP_STRING(
[--enable-debug], [build with debugging features enabled [default=no]])],
[], [])
AC_MSG_CHECKING([Build type])
if test x"$enable_debug" = "xyes"; then
BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE"
CFLAGS="-g -O0"
AC_MSG_RESULT([debug])
else
AC_MSG_RESULT([release])
fi
#-----------------------------------------------------------------------
```
It adds three preprocessor definitions and overrides `CFLAGS` with `"-g -O0"`. The latter breaks the user's ability to provide sanitizer and LTO flags.
This PR might be especially useful for OSS-Fuzz where `DEBUG=1` has been used since https://github.com/google/oss-fuzz/pull/10503.
Also it makes a workaround for building SQLite for 32-bit unneeded. For details, please refer to https://github.com/hebasto/oss-fuzz/tree/240120-sqlite.
Changes in https://github.com/bitcoin/bitcoin/pull/29282 might not be strictly required now. However, I consider them an improvement.
ACKs for top commit:
fanquake:
ACK 5fb8f0f80f - downstream is also green, so i'll fixup the PR there.
Tree-SHA512: 8593d8a0237ebb270d5da763fb65ed642ab8ed0d44e57704a34154621f49e3d5c58b462cc0070251fa1ba556c58a3c7d3620530d6839dc6dc9e0887010330eca
The `--enable-debug` configure option for the SQLite package does two
things. It adds three preprocessor definitions and overrides CFLAGS with
"-g -O0". The latter breaks the user's ability to provide sanitizer and
LTO flags.
