fabd34873c test: Rename EncodeDecimal to serialization_fallback (MarcoFalke)
Pull request description:
The new name better explains that the function handles fallbacks, without listing all in the function name.
ACKs for top commit:
theStack:
ACK fabd34873c
Tree-SHA512: a0405aab2bfb2fd10c61b51b4eb767053b25b0d914d2dac006dd3eaf360fbc6f3a444bc7b580ab8469ec492fe4358cfad5943adde4a7c8f783032ceef5cc5383
11900e5a8a doc: simplify the router options in doc/i2p.md (Jon Atack)
b505d59326 doc: clarify when and how to launch the SAM bridge in doc/i2p.md (Jon Atack)
Pull request description:
1. Clarify when and how to launch the SAM application bridge to address user questions and https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1599449753. The bridge is not enabled by default in the Java I2P Router, and the relevant info is somewhat difficult to find in its documentation.
2. Remove a duplicate sentence and link (the preceding paragraph begins with the same sentence and link).
3. Simplify the router options:
- the Java I2P router and i2pd are the two routers have been heavily tested with Bitcoin Core and are what node operators and node software packages are using
- [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 and I haven't heard anyone report using them
ACKs for top commit:
fanquake:
ACK 11900e5a8a
Tree-SHA512: add5f40004feddc7829d658a5de30422f144ab8fa09ff1396d5d263ee86e39803595a2c50e2aa601d0df6da5eff4358c3f7f17e039dcd4952cb306596c6c0397
fa086248e5 test: Use same timeout for all index sync (MarcoFalke)
Pull request description:
Seems odd to use different timeouts.
Fix this by using the same timeout for all syncs.
May also fix https://github.com/bitcoin/bitcoin/issues/27355 or at least make it less frequent?
ACKs for top commit:
mzumsande:
code review ACK fa086248e5
Tree-SHA512: a61619247c97f3a88dd19eb3f200adedd120e6da8c4e4f2cf83621545b8c289dbad77e16f13cf7973a090f7b2c3391cb0297f09b0cc95fe4f55de21ae247670f
3210f224db refactor: remove in-code warning suppression (fanquake)
Pull request description:
Should no-longer be needed post #27872. If it is, then suppress-external-warnings should be fixed.
ACKs for top commit:
hebasto:
ACK 3210f224db
Tree-SHA512: 2405250b7308779d576f13ce9144944abd5b2293499a0c0fe940398dae951cb871246a55c0e644a038ee238f9510b5845c3e39f9658d9f10225a076d8122f078
5fa4055452 net: do not `break` when `addr` is not from a distinct network group (brunoerg)
Pull request description:
When the address is from a network group we already caught,
do a `continue` and try to find another address until conditions
are met or we reach the limit (`nTries`).
ACKs for top commit:
amitiuttarwar:
utACK 5fa4055452
achow101:
ACK 5fa4055452
mzumsande:
utACK 5fa4055452
Tree-SHA512: 225bb6df450b46960db934983c583e862d1a17bacfc46d3657a0eb25a0204e106e8cd18de36764e210e0a92489ab4b5773437e4a641c9b455bde74ff8a041787
7c853619ee refactor: Drop unsafe AsBytePtr function (Ryan Ofsky)
Pull request description:
Replace calls to `AsBytePtr` with calls to `AsBytes` or `reinterpret_cast`. `AsBytePtr` is just a wrapper around `reinterpret_cast`. It accepts any type of pointer as an argument and uses `reinterpret_cast` to cast the argument to a `std::byte` pointer.
Despite taking any type of pointer as an argument, it is not useful to call `AsBytePtr` on most types of pointers, because byte representations of most types will be platform specific or undefined. Also, because it is named similarly to the `AsBytes` function, `AsBytePtr` looks safer than it actually is. Both `AsBytes` and `AsBytePtr` call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on `Span` objects, while `AsBytePtr` can be called on any pointer argument.
The change was motivated by discussion on #27973 and #27927 and is compatible with those PRs
ACKs for top commit:
jonatack:
re-ACK 7c853619ee
sipa:
utACK 7c853619ee
achow101:
ACK 7c853619ee
Tree-SHA512: 200d858b1d4d579f081a7f9a14d488a99713b4918b4564ac3dd5c18578d927dbd6426e62e02f49f04a3fa73ca02ff7109c495cb0b92bec43c27d9b74e2f95757
fae7c50d20 test: Run fuzz tests on macOS (MarcoFalke)
Pull request description:
Any reason not to?
ACKs for top commit:
jamesob:
Github ACK fae7c50d20
dergoegge:
utACK fae7c50d20
Tree-SHA512: e45122d73fafb17cea312258314b826cb0745e08daadd28465f687ec02d4c127d2f8cbe20179a9fff5712038850c02c968abb4838fa088b7555e28709317d3a3
a51d7abf1e guix: Specify symbols in modules explicitly (Hennadii Stepanov)
47d51fb048 guix: Drop unneeded modules (Hennadii Stepanov)
57fdedd0e9 guix: Unify fetch methods (Hennadii Stepanov)
Pull request description:
This PR cleans up the `contrib/guix/manifest.scm` in the following way:
- Unneeded for a successful build modules have be dropped.
- Some modules have been enhanced with `#:select` clauses, which improves maintainability (see the commit message for details).
ACKs for top commit:
TheCharlatan:
ACK a51d7abf1e
Tree-SHA512: 380a36d03ec303ff8700893cfaad75ca09d84a77fd08d6c6a1679ac96409014b36f0698eb071e09af25ad36f1bc62aec0eec1092146d879251c6a8cce586169b
8d9b90a61e Remove now-unnecessary poll, fcntl includes from net(base).cpp (Ben Woosley)
Pull request description:
As far as I can tell, the code calling for these includes was removed in:
6e68ccbefe#2435682d360b5a8#21387
ACKs for top commit:
fanquake:
ACK 8d9b90a61e
Tree-SHA512: e536d60f4cf204a10a5b461eca20c8329aa6b0fd3b27651ac9490ed42d3e22e31d652cd991ddc84c96e4758df15aefa7e7f84c710f2feb6d2e0fcfbda9ad4356
aaaa3aefbd test: Use TestNode *_path properties where possible (MarcoFalke)
dddd89962b test: Allow pathlib.Path as RPC argument via authproxy (MarcoFalke)
fa41614a0a scripted-diff: Use wallets_path and chain_path where possible (MarcoFalke)
fa493fadfb test: Use wallet_dir lambda in wallet_multiwallet test where possible (MarcoFalke)
Pull request description:
It seems inconsistent, fragile and verbose to:
* Call `get_datadir_path` to recreate the path that already exists as field in TestNode
* Call `os.path.join` with the hardcoded chain name or `self.chain` to recreate the TestNode `chain_path` property
* Sometimes even use the hardcoded node dir name (`"node0"`)
Fix all issues by using the TestNode properties.
ACKs for top commit:
willcl-ark:
re-ACK aaaa3aefbd
theStack:
Code-review ACK aaaa3aefbd🌊
Tree-SHA512: e4720278085beb8164e1fe6c1aa18f601558a9263494ce69a83764c1487007de63ebb51d1b1151862dc4d5b49ded6162a5c1553cd30ea1c28627d447db4d8e72
d4fb58ae8a test: EC: optimize scalar multiplication of G by using lookup table (Sebastian Falbesoner)
1830dd8820 test: add secp256k1 module with FE (field element) and GE (group element) classes (Pieter Wuille)
Pull request description:
This PR rewrites a portion of `test_framework/key.py`, in a compatible way, by introducing classes that encapsulate field element and group element logic, in an attempt to be more readable and reusable.
To maximize readability, the group element logic does not use Jacobian coordinates. Instead, group elements just store (affine) X and Y coordinates directly. To compensate for the performance loss this causes, field elements are represented as fractions. This undoes most, but not all, of the performance loss, and there is a few % slowdown (as measured in `feature_taproot.py`, which heavily uses this).
The upside is that the implementation for group laws (point doubling, addition, subtraction, ...) is very close to the mathematical description of elliptic curves, and this extends to potential future extensions (e.g. ElligatorSwift as needed by #27479).
ACKs for top commit:
achow101:
ACK d4fb58ae8a
theStack:
re-ACK d4fb58ae8a
stratospher:
tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!
Tree-SHA512: 9e0d65d7de0d4fb35ad19a1c19da7f41e5e1db33631df898c6d18ea227258a8ba80c893dab862b0fa9b0fb2efd0406ad4a72229ee26d7d8d733dee1d56947f18
Replace calls to AsBytePtr with direct calls to AsBytes or reinterpret_cast.
AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of
pointer as an argument and uses reinterpret_cast to cast the argument to a
std::byte pointer.
Despite taking any type of pointer as an argument, it is not useful to call
AsBytePtr on most types of pointers, because byte representations of most types
will be implmentation-specific. Also, because it is named similarly to the
AsBytes function, AsBytePtr looks safer than it actually is. Both AsBytes and
AsBytePtr call reinterpret_cast internally and may be unsafe to use with
certain types, but AsBytes at least has some type checking and can only be
called on Span objects, while AsBytePtr can be called on any pointer argument.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
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
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
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
