👍 rkrux approved a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2827881979)
tACK 1ee698fde2e8652d8eb083749edb8029c003b8db

💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081429882)
Given that `s[0] == 0` is asserted above and the signature is negated from `s[1]` onwards, I was trying to reason why this check is still needed before dropping `s[0]` & thought of the reason that if `s[0]` is dropped when `s[1] >= 0x80` then `s` will be treated as a negative number, which is illegal as mentioned in https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574.

💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081451063)
I checked that `ret` here means whether the passed `signature/seckey` in the second argument is valid according to `secp256k1_ec_seckey_verify`, which weirdly enough doesn't seem to be called by `secp256k1_ec_seckey_negate` - maybe the doc is outdated, but I get the general idea of validating the input & returning the response of this check.

https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/secp256k1/include/secp256k1.h#L689-L702

https://github.com/bitcoi
...

💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866148542)
> Not sure if this is worth it. Currently 4 lines of code need to be changed, looking at https://github.com/bitcoin/bitcoin/commit/b537a2c02a9921235d1ecf8c3c7dc1836ec68131. After this, 3 lines will need to be changed. I don't really see the benefit here.

Yeah, i more or less agree. It's already been condensed to one place, maybe that's enough.

💬 laanwj commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2081461596)
Right.

It'd be possible to structure this slightly more intuitive, by first removing the first byte (after asserting it's 0x00), then doing `secp256_blabla` on the entire 32-byte thing, then conditionally readding it after.

But meh. It's not like this is a easty-to-digest function in the first place 😓

✅ fanquake closed a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)

💬 laanwj commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2866165846)
> The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.

i think this makes sense, especially for the symbol check. The security check may have some value as a more general "binary security check". But moving both to a guix-specific path would be fine with me.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476373)
I admit defeat, added:
```C++
/** Generate random bytes on the stack. */
template <BasicByte B = unsigned char, size_t N>
std::array<B, N> randbytes() noexcept
{
std::array<B, N> ret;
Impl().fillrand(MakeWritableByteSpan(ret));
return ret;
}
```

See: a66c1841bd5708eb446c805d6612e2448d6022f3

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081476663)
Some compilers had problem with string `constexpr`, it's why I inlined it instead.
I don't mind reverting to `OBFUSCATION_KEY_KEY` (note `OBFUSCATE` -> `OBFUSCATION`), if you think it's better_better.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081477824)
Thanks

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2866204024)
There was a seemingly intermittent CI failure in wallet_basic.py on the commit merging this into master. Tracked as #32456. The following push to master succeeded.

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081485161)
Thanks for clarifying. I got the impression that you did think it was useful from "It may be a good idea to actually improve the check here, since it's relied on by `getblocktemplate` in proposal mode and by #31981" (https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2053597806). But I guess you were saying it *could* be useful if it were improved, not that it is useful currently.

Anyway I was mostly asking about this because I was confused. Assuming you are happy with PR in current fo
...

💬 carnhofdaki commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2866215127)
Concept ACK, the patches look fine to me. Compiling 92b05c17b1 right now. Thanks for doing it!

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2081493334)
Good catch! I do think daemon could be ok but I agree node is better. I will update this (and I also want to look into @theStack's suggestion to use subprocess.h, which I was unaware of and could simplify the PR)

💬 laanwj commented on pull request "randomenv: remove some `/proc/` accesses":
(https://github.com/bitcoin/bitcoin/pull/32450#issuecomment-2866302918)
> E.g. simplest implementation: pass a reference to a static flag to every AddFile( and set it to false after a failure. If it's false at start, don't try again.

Alternative using command pattern:
```diff
diff --git a/src/randomenv.cpp b/src/randomenv.cpp
index fbab23afe94315faa65d58d15ee0856875e1e92c..411985c662ea904eeaa983ed87e7597dd1485509 100644
--- a/src/randomenv.cpp
+++ b/src/randomenv.cpp
@@ -94,10 +94,29 @@ void AddSockaddr(CSHA512& hasher, const struct sockaddr *addr)
}
...

💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2081584114)
This behaviour is MSVC-specific (I've added a note to the PR description). And I cannot reproduce it when running cross-compiled binaries.

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081584805)
It can be, and that was why the 1 second timeout check kept failing. I tried a few implementations of the test and it's just impossible to reliably start the test timer in sync with the HTTP server. So sometimes it starts late and you end up with `duration < expected`.

The point of the test is to ensure that the server disconnects idle clients, I think it does that even though the exact time is a little fudged.

💬 hebasto commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2866400462)
@laanwj

Are you going to upstream the second commit?

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2866412296)
> Are you going to upstream the second commit?

Yes, but only after cpp-subprocess starts using c++17: https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844382194

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2081597743)
Was done on purpose - to retire the type of `Obfuscation` gradually - but I don't mind doing it early either.
Changed in the commit messages as well.