💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302746644)
@sipa sure, but if we're weighing a change to this, it still matters as it may modify how many iterations we're willing to do

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2251681805)
reviewed through bbec8e261467637ce26ecf551b528d9061cf4ffe

via `git range-diff master 388c2f5d7ee5f38cbefaff0c85cf298083127299 bbec8e261467637ce26ecf551b528d9061cf4ffe`

LGTM aside from waiting on fuzzer coverage results + fixes

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725592055)
nit: just assert it's in orphanage instead

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302806417)
reACK 12760a57b3a6cd1aeb3b7532311f648de2b45aa4

via `git range-diff master 1b8f143fb791fe5ba40ce6610cd4e8e04b951495 12760a57b3a6cd1aeb3b7532311f648de2b45aa4`

💬 fjahr commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2302873162)
ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

Reviewed the code and verified that the newly added tests do actually test the change in behavior.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725663019)
Not sure about this. To catch the seed, logging should be enabled (and it is enabled in all CI tasks). (This isn't changed in this pull request, so the comment seems tangential)

If the burden is put on each test that uses any kind of randomness to capture and memorize the seed, and print it on failure, it will be spotty at best and confusing, verbose and inconsistent in general.

If you are worried that the seed could be missed, I share your concern, but a better or more general fix would b
...

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2302894655)
> Concept NACK on the approach here (CI is showing why it wont work with depends, I assume it will also break other builds (where we haven't built the libs)). I'd rather build up from from something like #30438 (and have a branch with similar changes).

1. CI is green.
2. This approach works and fixes [#30677](https://github.com/bitcoin/bitcoin/issues/30677).
3. [#30438](https://github.com/bitcoin/bitcoin/issues/30438) does not indicate that it is capable of resolving [#30677](https://github
...

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725675521)
Personally, I like how it is done in the python functional tests:

* Allow to log it to the terminal (This can be used by developers)
* Always record the seed in the log in the test temp dir, which is preserved on failure, so that it can be looked up afterwards. (This is used by CI and can be used by developers as well)

Doing the same in the unit tests would be nice, but again, this seems best to be tracked, discussed and fixed in a separate issue or pull request.

💬 sipa commented on issue "Control-flow application capabilities for `x86_64-linux-gnu` release binaries":
(https://github.com/bitcoin/bitcoin/issues/30677#issuecomment-2302905383)
Does the ELF flag control whether the feature will be enabled at runtime?

💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725686630)
I don't think this is the right place for these flags. If we want them in the Guix build, then we should add them to the Guix build, not hardcode them into depends.

👋 fanquake's pull request is ready for review: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558)

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2302919191)
> This might be a bug in LIEF. Opened https://github.com/lief-project/LIEF/issues/1082 upstream.

I haven't yet tested, but the bug should now be fixed, as of https://github.com/lief-project/LIEF/commit/ab85865f279cf02648018417ec8afa12bd0bef24.

💬 allanperlee 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_r1725693381)
I see your point, the issue was readability in this particular test, according to the issuer. Should I leave this alone and write a separate test? `signrawtransactionwithkey` is tested in other parts of this functional test.

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1725697479)
I just looked into this and from my understanding `CWallet::Flush()` delegates to the `Flush()` method of the underlying Database and that method is a [no-op in SQLite](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/sqlite.h#L155). Since we are moving to SQLite and remove BDB entirely my guess is that the `Flush()` method will be removed at some point in the future as well.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2302928839)
Thanks @ryanofsky, incorporated your latest feedback in 7a4d249267cb5f63ace96a5fcc03452acc5468b5.

💬 ryanofsky commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725702791)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725675521

That all sounds good but I think I'm proposing a smaller not bigger change in behavior by just fixing the bug instead of trying to do something grander. The change based on master is:

```diff
--- a/src/test/prevector_tests.cpp
+++ b/src/test/prevector_tests.cpp
@@ -209,9 +209,8 @@ public:
}

prevector_tester() {
- SeedRandomForTest();
- rand_seed = InsecureRand256();
- rand_cac
...

💬 sipa commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725703682)
Can you elaborate on how you think we should in general decide where configuration should live (default in configure/cmake vs depends vs guix)?

My thinking would be that depends builds are intended to be as close to production binaries as we can get them, but without having a deterministic build environment (so without controlling compiler or dependency versions).

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725690444)
Nit (feel free to disregard): Based on the description (`Minimum work assumed to exist on a valid chain in hex`), "chain" and "value" might be superfluous here
```suggestion
return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d characters hex"), *value, uint256::size() * 2)};
```

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725693175)
Since this is a "User" centric specialization of `FromHex` (and not a `Hex` specialization of `FromUser`), I'd keep the `FromHex` prefix, i.e.: `FromHexUser` or perhaps `FromHexLenient`, since the `User` part is just incidental

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725694129)
Could we extract this this as a constant and use it throughout this PR (e.g. in error messages and tests)?