💬 pinheadmz commented on pull request "[28.x] Backport #31407":
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2891009384)
Concept ACK, starting guix build of this branch and will try to codesign with certificate

👍 willcl-ark approved a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2850764439)
ACK e4aee0b3a23a9609e16bd7c8eda618dcf0a8b1db

💬 hebasto commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353)
> I don't think so; MSVC is _only_ complaining about consteval literals inside `( ? : )` ternaries. I've pushed another attempt to avoid it by moving the subexpressions out of the ternaries.

FWIW, this bug was fixed in VS 17.9.

See https://developercommunity.visualstudio.com/t/Ternary-operator-breaks-consteval/10150913.

💬 fanquake commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2891088686)
Concept ACK

📝 hebasto opened a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564)
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.

The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095777325)
> forgot to remove the include?

yes -_-

> However, the CLIENT_NAME symbol is still used in the kernel,

Mmh, that is done though the config header, and I am not sure how we should handle that yet. Seems like a separate problem to me? Then again the clientversion header is little more than some formatting helpers for some these variables. Could also remove `CLIENT_NAME` in the few cases where it is relevant, but `CLIENT_BUGREPORT` is probably useful to keep.

💬 hebasto commented on pull request "cmake: Add missed `SSE41_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2891147706)
> Just roll this into #32551?

Done.

✅ hebasto closed a pull request: "cmake: Add missed `SSE41_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/32550)

💬 hebasto commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2891148659)
Add a commit from https://github.com/bitcoin/bitcoin/pull/32550.

🤔 hodlinator reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2849775833)
Code review 9406c17223da9df3a9875e4a5769a96ae66d61b8

Getting pretty close to ACK. One main inlined question about error handling in dbwrapper.cpp.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095077056)
Should we really continue after reading an invalid key length? Feels like it would be good to throw an exception, or `assert` the condition instead of `if`.

(Could adjust the message/comment to include the possibility of us being old software running on some future data format we don't recognize?)

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095101679)
note in b085c52030ec30294f68abfdbe5b05acbae60a7e:
Seems like there's a newish footgun with curly-brace initialization of `vector`s.

This compiles because the passed value unambiguously specifies the length (`std::byte` isn't implicitly constructible from integer literals):
```
std::vector<std::byte> obfuscation{512};
```
The following fails, because it's interpreted as initializer-list construction, and the integer literal would be narrowed to fit into the first element of the `vector`:
...

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095154105)
nit: `0xDEADBEEF` is a 32-bit value. Might be more precise to use a 64-bit literal as above, or randomize?

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095188702)
nit: Should we use "old"/"new" qualifiers inline in the code? Feels more appropriate for something in the commit message.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095711047)
Thanks for lettering yourself be convinced! Guess I needed another round to sharpen my arguments too.

Nice that you didn't need the `span<uint8_t>` overload.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2095733067)
Slightly better_better, thanks!

* Why change it from being `private` to `public`, some later PR?
* nit: This compiles on GCC 14.2.1 and Clang 20.1.3:
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index ffb25b8ac1..ffe98a3fde 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -32,6 +32,11 @@
#include <optional>
#include <utility>

+// Prefixed with null character to avoid collisions with other keys
+//
+// We must use a string constructor which specifies len
...

💬 hebasto commented on pull request "Set minimum supported Windows version to 1903 (May 2019 Update)":
(https://github.com/bitcoin/bitcoin/pull/32537#issuecomment-2891173444)
> ... a version from more than 5 years ago...

6 :)

💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2095802755)
I'd say it would be best to keep all of them. Maybe I am missing the context, but I fail to see how the client version is irrelevant. "they can easily find its version when reporting the bug" doesn't sound great, because some people forget it, and it will just be another round-trip.

Also, I'd say it is important to know if the user ran some local (or remote) modifications. Lately it seems common to fork random commits of bitcoin core and apply random (policy or non-policy) patches. We'd want
...

👍 hebasto approved a pull request: "ci: Move DEBUG=1 to centos task"
(https://github.com/bitcoin/bitcoin/pull/32560#pullrequestreview-2850914158)
ACK fa58d6cdab000df288501db4a71487804b08ba4b.

👍 stickies-v approved a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2850770205)
re-ACK fa2445eea3e380386ab01631b735c7eeb6b76b58