💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709096542)
I think it would be nice to test some more invariants here:
```diff
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index aabc0650b8..fa5273446f 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -79,0 +80,3 @@ void IpcTest()
+ BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid());
+ BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError());
+ BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid());
@@ -82,0 +86,9 @@ void IpcTest()
+ BlockValidationState bs3;
+ B
...

👋 hebasto's pull request is ready for review: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454)

💬 Sjors commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275517691)
> Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed

This PR fixes the undefined behavior that the sanitizer picked up in #30514. I think that's sufficient for now. We can improve handling of corrupt and/or malicious files later, which (unless I missed something) is what @maflcko's critique is about.

Afaik any corrupt (accidentally or otherwise) snapshot file will ultimately be rejected
...

💬 Sjors commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1709153717)
51f197bd84916c494e9250926776b9efc3225100 maybe use `int32_t` instead of `int` for clarity?

```cpp
std::numeric_limits<int32_t>::max())
```

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2275538175)
@ryanofsky I updated the description.

In the earlier issue both @luke-jr (Knots) and @ajtowns (Inquisition) seemed to indicate that the change wouldn't be a problem for them. See https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903768404. Let me know if that's incorrect or if you changed your mind.

> not a big deal for the forks to modify this file if they want CI to run on branches they are modifying anyway

So far it seems I'm the only one using this and it's not bothering a
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275538629)
Force-pushed to address all outstanding review comments, mainly:
- [avoid string re-allocation](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707728879)
- [cleaned up](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707958805) `util/random.cpp` a bit more
- [removed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707734248) unnecessary docstring

🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2227455502)
Force-pushed to address all outstanding review comments, mainly:
- [avoid string re-allocation](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707728879)
- [cleaned up](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707958805) `util/random.cpp` a bit more
- [removed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707734248) unnecessary docstring

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709206741)
Would be nice to have std::optional support in BOOST_CHECK_EQUAL but this PR has already had quite a bit of churn so I'm going to limit the scope and leave as is given that it's not super relevant.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709194170)
Don't think this is worth spending too much time on but given that the only non-test call is quite likely to require padding, I've updated it to:

```diff
diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
index 4e0317cd0e..be946af269 100644
--- a/src/util/strencodings.cpp
+++ b/src/util/strencodings.cpp
@@ -8,6 +8,7 @@
#include <crypto/hex_base.h>
#include <span.h>

+#include <algorithm>
#include <array>
#include <cassert>
#include <cstring>
@@ -54,10 +55,8
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709207046)
I disagree, a227cb511ec948b37ddbc9ee65de586109ebc1da is a refactor commit so I prefer keeping the behaviour-changing commits such as e6f81b9d81359a7ffeff6d1830188f00df0e8db0 smaller and separate.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709211452)
> However, in the tests, personally I like to use it for brevity.

I agree. The tests fail gracefully, and the error message is helpful, including the location of the assertion which documents that we're expecting a 64 char hex string. Going to leave as is.

```
Running 1 test case...
test/util/random.cpp:29 operator(): Assertion `uint256::FromHex(num)' failed.
unknown location:0: fatal error: in "timeoffsets_tests/timeoffsets_warning": signal: SIGABRT (application abort requested)
test/
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709204523)
Agreed, autogenerated and didn't think to remove it. Done.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709155583)
I think you're right that `inline` was not necessary here. No longer applies as I've adopted [maflcko's suggestion](https://github.com/bitcoin/bitcoin/pull/30569/files#r1707958805) which moves the var back inside the `SeedRandomForTest` scope.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709153215)
Done, I've taken your commit (hadn't seen the commit message earlier, very nice) but just added the docstring (and smaller `num` scope) back in.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709162569)
This feels like a distraction to me tbh, we're not unit testing `ParseParameters` here. I think it's safe to rely on `ParseParameters` returning `false` when there's an error.

💬 maflcko commented on issue "Faster way to get block with prevouts in JSON-RPC":
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2275543824)
https://github.com/bitcoin/bitcoin/pull/30595 mentions "Traversing the block index as well and using block index entries for reading block and undo data." However, it does not return JSON-RPC, but a `kernel_BlockUndo*`/`BlockUndo`, also the pull is experimental, doesn't have versioning and has some other drawbacks. (Just mentioning it for context, because if you care about speed, this may be faster than JSON)

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1709228465)
@itornaza thanks, I'll look into your patch. Another guiding principle would be to stay close to `bip324.h` in terms of coding style. I haven't compared with that yet.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1709250026)
I'm not sure if we end using `getTxFees()` and `getTxSigops()` in stratum v2, or only for the existing stratum v1 RPC calls. Once sv2 code is a bit more mature it's indeed worth going over the interface once more.

💬 Sjors commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2275573473)
What you recall sounds similar to what still happens. But it seems this PR makes recovery from that failure easier (just delete the snapshot dir). I just don't fully understand why and it's very time consuming to reproduce on testnet3.

🤔 paplorinc reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2227399035)
Checked a few more scenarios on mac