Bitcoin Core Github
44 subscribers
120K links
Download Telegram
fanquake closed an issue: "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available""
(https://github.com/bitcoin/bitcoin/issues/31913)
🚀 fanquake merged a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437)
📝 fanquake opened a pull request: "doc: swap "Docker image" for "container image""
(https://github.com/bitcoin/bitcoin/pull/32444)
I haven't used Docker for some time (now Podman), and the images are generic, so just use "container image". I'll be pushing some changes to https://github.com/fanquake/core-review/tree/master/guix, to reflect this.
🤔 hodlinator reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2807054428)
Code review bba64732ffb6f9463bb1eced7953493935950cec.

Thanks for improving the dbwrapper test case and minimizing `_hex_v` usage in the end state of the PR!
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2068663871)
What is the point of changing this?

1. Renaming `OBFUSCATE_KEY_KEY` -> `OBFUSCATION_KEY` makes the first word nicer, but it is arguably the database key (1) for looking up the key (2) used to obfuscate (0) the data.
2. Making it `inline` might be shifting some work from the linker to the compiler (better for parallelism?). Having it all in the header is slightly nicer for humans, but seems like it would duplicate the constant where it is used. Could it just be declared as a file-local (`stat
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079132170)
nit in 366bffd1252a768e1161c7b632ef8c4816bb504e:
Could drop rename here from `Xor` -> `XorObfuscationBench` since next commit renames it again to `ObfuscationBench`.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079140446)
nit in 9712481ae727bb0d8ad392c5fc2980aacbd9091b: prefer verb for function, as you did for `DataStream::Obfuscate()`:
```diff
- inline void Obfuscation(std::span<std::byte> write, std::span<const std::byte> key, size_t key_offset = 0)
+ inline void Obfuscate(std::span<std::byte> write, std::span<const std::byte> key, size_t key_offset = 0)
```
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079094504)
nit in b9c54ccd8c4248e060c3b4186af0deb3b577d34f:
Could drop string conversions:
```suggestion
BOOST_CHECK_EQUAL(read_value, expected_value);
```
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079059244)
In b9c54ccd8c4248e060c3b4186af0deb3b577d34f:
nit, avoid repeated `dbwrapper_private::GetObfuscateKey` call:
```diff
- BOOST_CHECK(obfuscate != is_null_key(dbwrapper_private::GetObfuscateKey(dbw)));
- obfuscation_key = dbwrapper_private::GetObfuscateKey(dbw);
+ obfuscation_key = dbwrapper_private::GetObfuscateKey(dbw);
+ BOOST_CHECK_NE(obfuscate, is_null_key(obfuscation_key));
```
similarly on lines 68-69:
```diff
- BOOST_CHECK
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079245753)
The size of these vectors are treating `Obfuscation` with kid gloves, only to let it detonate in the wild. See https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053773631
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079237359)
Seems like this function is no longer referenced after this PR and could be dropped in the final commit.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079184969)
in 13cc039f20eae787ddbc00140dbc89a9a0b1ea05:
`CreateObfuscation` remains here despite the commit message.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2079218910)
In the `!key_missing` case, we've just read the `vector` from a file on disk. It could have been corrupted and got a wildly unexpected length. If it is too short we will read out of bounds here, if it is too long we will continue when we probably should be erroring out. See https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053773631
💬 laanwj commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2862367684)
Some more context: https://issues.guix.gnu.org/76503
📝 fanquake opened a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)
Then we no-longer have to "bump" it.
💬 fanquake commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2862377129)
cc @purpleKarrot
🤔 hebasto reviewed a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437#pullrequestreview-2824444216)
My Guix build:
```
aarch64
20608607794819e22c0bc02c920ba03a4edf98b902e278f1e2dcc9d92f2f485f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/SHA256SUMS.part
f3268558a957a2b55191b14bdb9f7c23d60dc72a84ecbfe6b56a280f8f09fd1f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu-debug.tar.gz
791e1b3e81b358343dd5fafa53ab1d503e819d6935515277443dcfef4091c470 guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu.tar.gz
1b689d1c
...
🤔 hebasto reviewed a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437#pullrequestreview-2824452024)
Post-merge ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8.
💬 laanwj commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#discussion_r2079302392)
i was worried for a bit here but apparently this appears in a comment only.
💬 hebasto commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2862397599)
> > Building our own depends implies that we fully rely on them, ideally skipping all other search paths used by CMake's package, library, and header search mechanisms. Therefore, I believe we should retain as many restrictions as possible.
>
> We may do that with a [dependency provider](https://cmake.org/cmake/help/latest/command/cmake_language.html#set-dependency-provider). This will put `find_package()` completely under our control, so restricting CMake's builtin find logic becomes unnecessa
...