💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709002249)
> Are you able to build them using Autotools?

Yea, the steps in `fuzzing.md` currently work with master, but don't work this branch (ad2140d4d8cccec82fdad07bdc7532e202282306).

💬 maflcko commented on pull request "tracing: explicitly cast block_connected duration to nanoseconds":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2275336617)
lgtm ACK 34c18de369b7bd71334f849395723f49dffae560

💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709005443)
We probably don't.

💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709030842)
I've only tried the instructions in this doc on linux when I wrote them a few weeks back. I'm guessing this is a Mac only issue but perhaps not.

💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709007912)
Right, the "-j$(nproc)" bit should be removed. I don't have a Mac so I don't no if these instructions are still relevant.

✅ hebasto closed a pull request: "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP"
(https://github.com/bitcoin/bitcoin/pull/29790)

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709076520)
The mac equivalent is `make -j$(sysctl -n hw.ncpu)`

💬 gmaxwell commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709093275)
Do these adhoc initialization costs interact well with the with the upper bound 2^(k-1) and the empirical sqrt(2^k)+1 limits in the tests or should the bounds in the test be increased by these adhoc initialization costs?

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2275425772)
Post-merge light re-utACK

Happy to see `enforce_BIP94` replace the hardcoded genesis block.

Followup suggestion: let's make `60 * 60 * 2` a constant and add a `static_assert` that it's equal to `MAX_FUTURE_BLOCK_TIME`. They are not the same thing, since `time-timewarp-attack` is (testnet4) consensus whereas `time-too-new` is not. But if we ever change one without changing the other, it can cause a chain split.

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708937001)
I'm having a hard time reading this TODO. Can you try and reformulate it?

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708943992)
Typo nit: s/canproto/capnproto/ (here and elsewhere).

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643)
Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040)
Not directly related to this PR, but did these type hook methods get renamed since the docs were written? https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/design/multiprocess.md#type-hooks-in-srcipccapnp-typesh

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709048212)
I'm a bit confused by this file. It is only included by `mining.capnp`, but in turn includes the auto-generated header from `mining.capnp` here. What is the purpose of this? Couldn't the capnp file just directly include `interfaces/mining.h`? At least doing so compiles for me.

💬 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