fa38d86235 Use only Span{} constructor for byte-like types where possible (MarcoFalke)
fa257bc831 util: Allow std::byte and char Span serialization (MarcoFalke)
Pull request description:
Seems odd to require developers to cast all byte-like spans passed to serialization to `unsigned char`-spans. Fix that by passing and accepting byte-like spans as-is. Finally, add tests and update the code to use just `Span` where possible.
ACKs for top commit:
sipa:
utACK fa38d86235
achow101:
ACK fa38d86235
ryanofsky:
Code review ACK fa38d86235. This looks great. The second commit really removes a lot of boilerplate and shows why the first commit is useful.
Tree-SHA512: 788592d9ff515c3ebe73d48f9ecbb8d239f5b985af86f09974e508cafb0ca6d73a959350295246b4dfb496149bc56330a0b5d659fc434ba6723dbaba0b7a49e5
Tests vectors were calculated by running the same tests on
v25. Which was the last release prior to introducing the
diff in the descriptor's string representation ('h' format).
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
As we update the descriptor's db record every time that
the wallet is loaded (at `TopUp` time), if the spkm ID differs
from the one in db, the wallet will enter in an unrecoverable
corruption state, and no soft version will be able to open
it anymore.
Because we cannot change the past, to stay compatible between
releases, we need to always use the apostrophe version for the
spkm IDs.
This allows us to verify the descriptor ID on the descriptors
unit tests in different software versions without requiring to
use the entire DescriptorScriptPubKeyMan machinery.
Note:
The unit test changes are introduced after the bugfix commit
but this commit + the unit test commit can be cherry-picked
on top of the v25 branch to verify IDs correctness. IDs must
be the same for v25 and after the bugfix commit.
If the computed descriptor's ID doesn't match the wallet's
DB spkm ID, return early from the loading process to prevent
DB data from being modified in any post-loading procedure
(e.g 'TopUp' updates the descriptor's data).
248a17addf ci: remove duplicate python3 from CI configs (fanquake)
b50767fdde ci: remove duplicate bsdmainutils from CI configs (fanquake)
Pull request description:
`bsdmainutils` and `python3` are included in `CI_BASE_PACKAGES`.
ACKs for top commit:
hebasto:
ACK 248a17addf
Tree-SHA512: 1e5cddd8a37128690ef3110891549cb9a4c69c6bca558137c97031bc0e494e1053063923d3ccee8b1d9f05d3432765ee10f9ce872e88959b802ba64b6e2d300c
This change improves the maintainability of the manifest:
(1) It allows to remove the module when the specified symbols are no
longer used.
(2) It prevents accidental use of other symbols, such as `bash`
instead of `bash-minimal`.
79d343a642 http: update libevent workaround to correct version (stickies-v)
Pull request description:
The libevent bug described in 5ff8eb2637 was already patched in [release-2.1.9-beta](https://github.com/libevent/libevent/releases/tag/release-2.1.9-beta), with cherry-picked commits [5b40744d1581447f5b4496ee8d4807383e468e7a](5b40744d15) and [b25813800f97179b2355a7b4b3557e6a7f568df2](b25813800f).
There should be no side-effects by re-applying the workaround on an already patched version of libevent (as is currently done in master for people running libevent between 2.1.9 and 2.1.12), but it is best to just set the correct version number to avoid confusion.
This will prevent situations like e.g. in https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1238858604, where a reverse workaround was incorrectly applied to the wrong version range.
ACKs for top commit:
fanquake:
ACK 79d343a642
Tree-SHA512: 56d2576411cf38e56d0976523fec951e032a48e35af293ed1ef3af820af940b26f779b9197baaed6d8b79bd1c7f7334646b9d73f80610d63cffbc955958ca8a0
529c92e837 guix: Update `python-lief` package to 0.13.2 (Hennadii Stepanov)
Pull request description:
The Guix's `python-lief` package is going to move to using external deps, rather than the bundled ones (https://lists.gnu.org/archive/html/guix-patches/2023-05/msg01302.html). We want to continue using our own package indefinitely, to keep the build simpler, and allow for easier updating.
Changes in `contrib/devtools/security-check.py` are caused by 6357c6370b.
Also see: https://github.com/bitcoin/bitcoin/pull/27507.
ACKs for top commit:
fanquake:
ACK 529c92e837
Tree-SHA512: ad81111b090a39b380fe25bb27b54a339e78a158f462c7adda25d5ee55f0d654107b1486b29b9687ad0808e27b01e04f53a0e8ffc6600b79103d6bd0dfec64ef
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This is done in addition with the following commit. Both have the goal
of getting rid of direct calls to AbortNode from kernel code. This extra
flushError method is added to notify specifically about errors that
arrise when flushing (syncing) block data to disk. Unlike other
instances, the current calls to AbortNode in the blockstorage flush
functions do not report an error to their callers.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
This is done in preparation for the next commit where a new FatalError
function is introduced. FatalErrorf follows common convention to append
'f' for functions accepting format arguments.
-BEGIN VERIFY SCRIPT-
sed -i 's/FatalError/FatalErrorf/g' $( git grep -l 'FatalError')
-END VERIFY SCRIPT-
This and the following commit seek to decouple the libbitcoinkernel
library from the shutdown code. As a library, it should it should have
its own flexible interrupt infrastructure without relying on node-wide
globals.
The commit takes the first step towards this goal by de-globalising
`ShutdownRequested` calls in kernel code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This change helps generalize shutdown code so an interrupt can be
provided to libbitcoinkernel callers. This may also be useful to
eventually de-globalize all of the shutdown code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
3c83b1d884 doc: Add release note for wallet loading changes (Andrew Chow)
2636844f53 walletdb: Remove loading code where the database is iterated (Andrew Chow)
cd211b3b99 walletdb: refactor decryption key loading (Andrew Chow)
31c033e5ca walletdb: refactor defaultkey and wkey loading (Andrew Chow)
c978c6d39c walletdb: refactor active spkm loading (Andrew Chow)
6fabb7fc99 walletdb: refactor tx loading (Andrew Chow)
abcc13dd24 walletdb: refactor address book loading (Andrew Chow)
405b4d9147 walletdb: Refactor descriptor wallet records loading (Andrew Chow)
30ab11c497 walletdb: Refactor legacy wallet record loading into its own function (Andrew Chow)
9e077d9b42 salvage: Remove use of ReadKeyValue in salvage (Andrew Chow)
ad779e9ece walletdb: Refactor hd chain loading to its own function (Andrew Chow)
72c2a54ebb walletdb: Refactor encryption key loading to its own function (Andrew Chow)
3ccde4599b walletdb: Refactor crypted key loading to its own function (Andrew Chow)
7be10adff3 walletdb: Refactor key reading and loading to its own function (Andrew Chow)
52932c5adb walletdb: Refactor wallet flags loading (Andrew Chow)
01b35b55a1 walletdb: Refactor minversion loading (Andrew Chow)
Pull request description:
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So this PR changes the way we load records by choosing to load the records in a particular order. This lets us do things such as loading a descriptor record, then finding and loading that descriptor's cache and key records. In the future, this will also let us use `IsMine` when loading transactions as then `IsMine` will actually be working as we now always load keys and descriptors before transactions.
In order to get records of a specific type, this PR includes some refactors to how we do database cursors. Functionality is also added to retrieve a cursor that will give us records beginning with a specified prefix.
Lastly, one thing that iterating the entire database let us do was to find unknown records. However even if unknown records were found, we would not do anything with this information except output a number in a log line. With this PR, we would no longer be aware of any unknown records. This does not change functionality as we don't do anything with unknown records, and having unknown records is not an error. Now we would just not be aware that unknown records even exist.
ACKs for top commit:
MarcoFalke:
re-ACK 3c83b1d884🍤
furszy:
reACK 3c83b1d8
ryanofsky:
Code review ACK 3c83b1d884. Just Marco's suggested error handling fixes since last review
Tree-SHA512: 15fa56332fb2ce4371db468a0c674ee7a3a8889c8cee9f428d06a7d1385d17a9bf54bcb0ba885c87736841fe6a5c934594bcf4476a473616510ee47862ef30b4
32e2ffc393 Remove the syscall sandbox (fanquake)
Pull request description:
After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).
There is more related discussion in #24771.
Note that given where it's used, the sandbox also gets dragged into the kernel.
If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.
Closes#24771.
ACKs for top commit:
davidgumberg:
crACK 32e2ffc393
achow101:
ACK 32e2ffc393
dergoegge:
ACK 32e2ffc393
Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
- the Java I2P router and i2pd are the two routers have been heavily tested
with Bitcoin Core and are what people and node software packages use
- i2p-zero (https://github.com/i2p-zero/i2p-zero) hasn't been updated since
July 2021 and its last release was in December 2020
- the other routers in the wikipedia page are niche
The SAM application bridge is not enabled by default in the Java I2P Router,
and the relevant info is somewhat difficult to find in its documentation.
Also, remove a duplicate sentence; the preceding paragraph begins with the same.
Instead of iterating the database to load the wallet, we now load
particular kinds of records in an order that we want them to be loaded.
So it is no longer necessary to iterate the entire database to load the
wallet.
Instead of dealing with these records when iterating the entire
database, find and handle them explicitly.
Loading of OLD_KEY records is bumped up to a LOAD_FAIL error as we will
not be able to use these types of keys which can lead to users missing
funds.
Instead of loading active spkm records as we come across them when
iterating the database, load them explicitly.
Due to exception handling changes, deserialization errors are now
treated as critical.
Instead of loading address book records as we come across them when
iterating the database, load them explicitly
Due to exception handling changes, deserialization errors are now
treated as critical.
The error message for noncritical errors has also been updated to
reflect that there's more data that could be missing than just address
book entries and tx data.
Instead of loading descriptor wallet records as we come across them when
iterating the database, loading them explicitly.
Exception handling for these records changes to a per-record type basis,
rather than globally. This results in some records now failing with a
critical error rather than a non-critical one.
Instead of loading legacy wallet records as we come across them when
iterating the database, load them explicitly.
Exception handling for these records changes to a per-record type basis,
rather than globally. This results in some records now failing with a
critical error rather than a non-critical one.
On my machine, this speeds up the functional test feature_taproot.py by
a factor of >1.66x (runtime decrease from 1m16.587s to 45.334s).
Co-authored-by: Pieter Wuille <pieter@wuille.net>
5fc4939e17 Added static_assert to check that base_blob is using whole bytes. (Brotcrunsher)
Pull request description:
Prior to this commit it was possible to create base_blobs with any arbitrary amount of bits, like base_blob<9>. One could assume that this would be a valid way to create a bit field that guarantees to have at least 9 bits. However, in such a case, base_blob would not behave as expected because the WIDTH is rounded down to the closest whole byte (simple integer division by 8). This commit makes sure that this oddity is detected and blocked by the compiler.
ACKs for top commit:
MarcoFalke:
lgtm ACK 5fc4939e17
theStack:
ACK 5fc4939e17
stickies-v:
ACK 5fc4939e17
Tree-SHA512: 6a06760f09d4a9e6f0b9338d4dddd4091f2ac59a843a443d9302959936d72c55f7cccd55a51ec3a5a799921f68be1b87968ef3c9c11d3389cbd369b5045bb50a
3df6070466 contrib: remove macOS lazy_bind check (fanquake)
9bc357e205 build: explicitly opt-in to new fixup_chains functionality for darwin (Cory Fields)
fb61bc0c02 depends: Bump MacOS minimum runtime requirement to 11.0 (Cory Fields)
c2cd47280c depends: bump darwin clang to 11.1 (Cory Fields)
Pull request description:
This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](https://github.com/bitcoin/bitcoin/pull/21778) for building Darwin binaries.
For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well.
The commits may seem unrelated, so in detail:
lld (llvm's linker) has been a work-in-progress for Darwin for years. Recently though, it has gained nearly all of the features we require. The last missing feature from ld64, `-Wl,-bind_at_load`, is not implemented in lld; as far as I can tell [lazy loading has conceptually been replaced by fixup chains](https://www.emergetools.com/blog/posts/iOS15LaunchTime).
So that means we don't need ld64's `bind_at_load` as long as lld can handle `-Wl,-fixup_chains` (which it can). I've added it to our configure as a linker option mostly so that we can see it in the logs; it's default-on as long as the minimum version is >11.0.
About that: the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp#L1021). Hence the commit that bumps the minimum version. Our current min runtime of `10.15` has been unsupported since September 2022, so I don't expect this bump to be controversial.
Lastly, with the minimum runtime version bumped to 11.0, our current version of pre-compiled clang we use for macOS is too old to understand `-mmacosx-version-min=11.0` because it expects `=10.x`. So I've made the smallest possible bump (from 10.0.1 to 11.1.0) to a version that understands. This bump is arbitrary and unfortunate, but likely to be short-lived as we may end up replacing it with llvm+lld for v26 anyway. I've held off on bumping the SDK as I think that makes sense to do as part of the lld switch instead.
ACKs for top commit:
hebasto:
ACK 3df6070466
gruve-p:
ACK 3df6070466
fanquake:
ACK 3df6070466
TheCharlatan:
ACK 3df6070466
Tree-SHA512: 0200ec4a3b88df33877ae82c15b5c04d745852550923f491a354b391cac65f88e4df116a40055c23a8cbcfcdfb9a376c6ae8fdd0e898e7b966bc213dcb5857cf
If the user used a custom change address, it may not be detected as a
change output, resulting in an additional change output being added to
the bumped transaction. We can avoid this issue by allowing the user to
specify the position of the change output.
3168b08043 Bench test for EllSwift ECDH (Pieter Wuille)
42d759f239 Bench tests for CKey->EllSwift (dhruv)
2e5a8a437c Fuzz test for Ellswift ECDH (dhruv)
c3ac9f5cf4 Fuzz test for CKey->EllSwift->CPubKey creation/decoding (dhruv)
aae432a764 Unit test for ellswift creation/decoding roundtrip (dhruv)
eff72a0dff Add ElligatorSwift key creation and ECDH logic (Pieter Wuille)
42239f8390 Enable ellswift module in libsecp256k1 (dhruv)
901336eee7 Squashed 'src/secp256k1/' changes from 4258c54f4e..705ce7ed8c (Pieter Wuille)
Pull request description:
This replaces #23432 and part of #23561.
This PR introduces all of the ElligatorSwift-related changes (libsecp256k1 updates, generation, decoding, ECDH, tests, fuzzing, benchmarks) needed for BIP324.
ElligatorSwift is a special 64-byte encoding format for public keys introduced in libsecp256k1 in https://github.com/bitcoin-core/secp256k1/pull/1129. It has the property that *every* 64-byte array is a valid encoding for some public key, and every key has approximately $2^{256}$ encodings. Furthermore, it is possible to efficiently generate a uniformly random encoding for a given public key or private key. This is used for the key exchange phase in BIP324, to achieve a byte stream that is entirely pseudorandom, even before the shared encryption key is established.
ACKs for top commit:
instagibbs:
reACK 3168b08043
achow101:
ACK 3168b08043
theStack:
re-ACK 3168b08043
Tree-SHA512: 308ac3d33e9a2deecb65826cbf0390480a38de201918429c35c796f3421cdf94c5501d027a043ae8f012cfaa0584656da1de6393bfba3532ab4c20f9533f06a6
11d650060a feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee (Andrew Chow)
Pull request description:
Returning the sats/kvb does not need to round trip through GetFee(1000) since the feerate is already stored as sats/kvb.
Fixes#27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.
ACKs for top commit:
furszy:
Code ACK 11d65006
Tree-SHA512: bec1a0d4b572a0c810cf7eb4e97d729d67e96835c2d576a909f755b053a9707c2f1b3df9adb8f08a9c4d310cdbb8b1e1b42b9c004bd1ade02a07d8ce9e902138
54877253c8 test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes#27595) (Sebastian Falbesoner)
Pull request description:
The functional test feature_taproot.py fails in some rare cases on the execution of the following `"branched_codesep"` spending script (can be reproduced via `$ ./test/functional/feature_taproot.py --randomseed 9048710178866422833` on master / 137a98c5a2):
9d85c03620/test/functional/feature_taproot.py (L741)
The problem occurs if the first data-push (having random content with a random length in the range [0, 510]) has a length of 1 and the single byte has value of [1...16] or [-1]; in this case, the data-push is not minimally encoded by test framework's CScript class (i.e. doesn't use the special op-codes OP_1...OP_16 or OP_1NEGATE) and the script interpreter throws an SCRIPT_ERR_MINIMALDATA error:
```
test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Data push larger than necessary) (-26)
```
Background: the functional test framework's CScript class translates passed bytes/bytearrays always to data pushes using OP_PUSHx/OP_PUSHDATA{1,2,4} op-codes (see `CScript.__coerce_instance(...)`). E.g. the expression `CScript(bytes([1]))` yields `bytes([OP_PUSH1, 1])` instead of the minimal-encoded `bytes([OP_1])`.
Fix this by adapting the random-size range to [2,...], i.e. never pass byte-arrays below length two to be pushed.
Closes#27595.
ACKs for top commit:
instagibbs:
ACK 54877253c8
sipa:
utACK 54877253c8
achow101:
ACK 54877253c8
Tree-SHA512: 3ffad89b2c3985c20702242192e744c9b10188bff880efaf3c38424a00fa07bd4608d8c948678ff9cdbb4e1e5b06696c7f55407ee10bb05edbb3ee03aa599cdc
77d6d89d43 net: net_processing, add `ProcessCompactBlockTxns` (brunoerg)
Pull request description:
When processing `CMPCTBLOCK` message, at some moments we can need to process compact block txns / `BLOCKTXN`, since all messages are handled by `ProcessMessage`, so we call `ProcessMessage` all over again.
ab98673f05/src/net_processing.cpp (L4331-L4348)
This PR creates a function called `ProcessCompactBlockTxns` to process it to avoid calling `ProcessMessage` for it - this function is also called when processing `BLOCKTXN` msg.
ACKs for top commit:
instagibbs:
reACK 77d6d89d43
ajtowns:
utACK 77d6d89d43
achow101:
ACK 77d6d89d43
Tree-SHA512: 4b73c189487b999a04a8f15608a2ac1966d0f5c6db3ae0782641e68b9e95cb0807bd065d124c1f316b25b04d522a765addcd7d82c541702695113d4e54db4fda
