💬 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
...

💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862408273)
Concept ACK but NACK on the implementation, as i understand this makes the build output depend on the system clock.

What about making the COPYRIGHT_YEAR depend on the last git commit?

🤔 i-am-yuvi reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2824481927)
re-ACK 69139049194e55a7fd99b7125b9afccb8585922d

💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862417346)
> What about making the COPYRIGHT_YEAR depend on the last git commit date in UTC?

I'm open to implementing this in any way, but if the logic for doing so, is going to end up being more than a few lines / add other requirements (i.e what happens if you're building from a tarball/no git etc), then maybe we shouldn't bother. Just wanted to -1 the number of things that need maintenance.

📝 fanquake converted_to_draft a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)
Then we no-longer have to "bump" it.