💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2304465368)
I like it a lot more now, thanks for considering the feedbacks.

The only part that still bugs me a bit is the naming of `FromUserHex` -> the "user" is on a different abstraction level, I'd prefer mentioning the behavior change compared to the sibling method (i.e. it's more forgiving/leninent for the inputs), rather than the motivation for adding the method (the users sometimes prefer different formats).

ACK 81554aac80bf2270db977c110c37acc7e8034194

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726880388)
nit, if you touch again, consider:
```suggestion
std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";
```

💬 l0rinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726907251)
we're adding some follow-ups as PRs to https://github.com/hebasto/bitcoin/pulls

💬 Sjors commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2304492506)
Getting the same guix hashes as @hebasto and x86_64.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726922698)
> we're adding some follow-ups as PRs to https://github.com/hebasto/bitcoin/pulls

Right. This, and https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs+CMake+port%22+is%3Aclosed.

💬 l0rinc commented on pull request "Fix maybe-uninitialized warning in IsSpentKey":
(https://github.com/bitcoin/bitcoin/pull/30691#issuecomment-2304495811)
utACK 17707db939cb09a16c002d226152e71fce9289f2

👍 BrandonOdiwuor approved a pull request: "Fix maybe-uninitialized warning in IsSpentKey"
(https://github.com/bitcoin/bitcoin/pull/30691#pullrequestreview-2254358116)
Code Review ACK 17707db939cb09a16c002d226152e71fce9289f2

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726964446)
```
< snip >
x86_64-linux-gnu-ld: /tmp/ccDAU1BA.o: error: missing IBT and SHSTK properties
collect2: error: ld returned 1 exit status
E
======================================================================
ERROR: test_ELF (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/distsrc-base/distsrc-059bcf847da0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 67, in test_ELF
se
...

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726977836)
```
x86_64-linux-gnu-ld: /tmp/ccwqGS4u.o: error: missing IBT and SHSTK properties
collect2: error: ld returned 1 exit status
E
======================================================================
ERROR: test_ELF (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/distsrc-base/distsrc-5fda052191b0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 67, in test_ELF
self.assertE
...

💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726985551)
GIven that the whole point of these scripts is to sanity check the (release) tests, using the release flags, and in the Guix env; I don't think an undocumented workaround, of just hiding certain release flags, from the test scripts, is the right thing to do.

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726997573)
OK. What can you suggest?

💬 ryanofsky commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1727012811)
re: https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1725697479

I think that's right, but I don't see these sqlite or bdb implementation details being relevant here. My point is that before unloading a wallet, and before backing up a wallet, it would seem like a good idea to flush wallet state that's in memory to disk, so information is not lost. So it would seem logical to do that in the Flush method and to call Flush before backing up the wallet.

💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1727016049)
Comparing the output of `PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 src/test/fuzz/fuzz` for both autotools and cmake can be used to verify that all harnesses are ported.

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2304631078)
Doesn't look like `ReceivedTx` ever returns a package to validate in any of the fuzz tests (500 CPU hours each):

* http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-pr30110/harnesses/txdownload_impl/coverage_report/coverage/workdir/bitcoin/src/test/fuzz/txdownloadman.cpp.html#L352
* http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-pr30110/harnesses/txdownloadman/coverage_report/coverage/workdir/bitcoin/src/test/fuzz/txdownloadman.cpp.html#L213
* http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-pr3
...

🤔 glozow reviewed a pull request: "Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py`"
(https://github.com/bitcoin/bitcoin/pull/30667#pullrequestreview-2254476437)
I'm going to close this pull request for now. If you'd like to work on the issue, here are some hints before you open a new pull request.

1. Please ensure your code works before submitting a pull request. There is a lot of documentation for the functional test suite if you need help https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md
2. Write code a bit more carefully. `node` isn't an attribute; you forgot an `s`. `MiniWallet` does not have a `sendrawtransaction` functio
...

💬 glozow commented on pull request "Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#discussion_r1727027744)
Ah apologies, I was too hasty and didn't notice that this is for the funding transactions. You can ignore that.

✅ glozow closed a pull request: "Refactor: MiniWallet functionality for more readable code in functional test `rpc_signtransactionwithkey.py`"
(https://github.com/bitcoin/bitcoin/pull/30667)

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2304734471)
> Doesn't look like ReceivedTx ever returns a package to validate in any of the fuzz tests (500 CPU hours each):

At a minimum `TESTED_TX_RESULTS` is missing `TxValidationResult::TX_RECONSIDERABLE`, so nothing is ever failing for the right reason to try a package.

I restricted the set of possible transactions down to just a parent/child combo in TRANSACTIONS and was able to trigger it quickly, so it's likely reachable with that one change.

💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727105521)
In commit b4d37fa70506129ba4a4d218bc49ce8ca9d8380c

I think there is a significant change in behaviour here between `waitTipChanged`, or rather `g_best_block`, and the rpc notification through `blockTip`. AFAICT the `blockTip` notification reports on completed operations in `ActivateBestChain` and `InvalidatedBlock`, while `g_best_block` is updated through the lower-level `ConnectTip` and `DisconnectTip`, which may respectively be called iteratively by `ActivateBestChain` and `InvalidateBlock`
...

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2304745347)
The PR description has been updated with a runtime check example.