💬 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

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709170006)
Nit:
```suggestion
-DCMAKE_C_COMPILER=/usr/pkg/gcc12/bin/gcc \
-DCMAKE_CXX_COMPILER=/usr/pkg/gcc12/bin/g++ \
```

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709105914)
nit, what's the purpose of the comment?
Is it to explain to the reviewers how to review it or will this make sense after we remove autotools? If the second, maybe we can explain in the commit message instead - or add a TODO in front of this to signal that it's a temporary explanation (otherwise people will be afraid to remove it),

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709206550)
nit: not very important, but can help with avoiding merge conflicts if we always sort these alphabetically (would be nice to have an automatic check for these, of course).
Unless this is deliberate, in which case a comment could help.

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709172104)
Nit: to be a bit more consistent, we may want to add quotes only when they're needed (or always)
```suggestion
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
```

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709187478)
nit: slightly unrelated, but in other cases similar code blocks are annotated as either "\```bash" or "```sh" which are treated a bit different in e.g. CLion:
<img src="https://github.com/user-attachments/assets/13f30a0a-1f92-4a39-87ce-8ca11a05a44f">

Also note that "\``` bash" is also a formatted differently than "```bash".

Should probably be done in a different PR, though...

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709167244)
nit: "Python 3" is a bit confusing here since Python 2 was sunset in 2020, and we don't accept all 3.x versions.
Would it make sense to simplify these "Python 3" references to just "Python"?

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709145204)
👍 GUI is working with cmake, but I had to execute a `brew link qt5 --force` as well after `brew install qt@5` (applies to Autotools as well)

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709278334)
Is there any way I could help here?

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709270807)
Wanted to suggest`"([A-Za-z0-9]{2})"` (or `\w{2}`), but it seems the regex parser was written almost 40 years ago https://github.com/Kitware/CMake/blob/master/Source/kwsys/RegularExpression.cxx#L628

💬 Sjors commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2275621895)
Would be quite nice to get rid of openssl-1.1.1 entirely. If the upstream is merged and adopted here, I might nuke my Guix setup to try it.

💬 maflcko commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275639826)
> This PR fixes the undefined behavior that the sanitizer picked up in #30514.

I don't think this is UB. Personally I couldn't care less about a implicit fuzz integer issue, and I don't think any production user cares either, because fuzz tests aren't run in production.

I don't see the value in only silencing the fuzz target, but leaving the other reported bugs unfixed. If silencing the fuzz target was a goal, it could be done with a trivial temporary one-line patch to the suppressions fil
...