👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2248351160)
Code review ACK c3fa29db1b05aa51f74cc8be5bdae59be4f3b7c0. Main changes since last review were cleanup commits being splitup, and ToScript being changed to accept std::byte so it works better with HexLiteral

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723616777)
In commit "refactor: Hand-replace some ParseHex -> util::HexLiteral" (288f995f71fd8e6f750d668e47237e64d893cc1f)

I feel like commit message could benefit from a summary of the changes it is making. Maybe would suggest:

- The following scripted-diff commit will replace `ParseHex(...)` with `HexLiteral<uint8_t>(...)` but replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte can be used instead of uint8_t, so make replacements
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723537853)
In commit "refactor: de-Hungarianize CCrypter" (7713b5fb5c5bf0897037b9857a4adfbbc7e8db56)

Commit message seems to suggest that this is only changing names and adding braces, but this is also switching an iterator type to auto. Might be good to mention in commit message, assuming this change wasn't intended for a different commit.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723523414)
In commit "refactor: add util::HexLiteral and util::Vec using statements" (af751d7030ccb5c2e3e699bd90655a53529953a3)

Renaming `container` variable to `span` would seem clearer, since this is now a span not a generic container.

Also this commit message is out of date since it is no longer using util::Vec, and the commit is no longer mostly about adding using statements. I'd probably just call it "Prepare for HexLiteral scripted-diff"

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1723632249)
What you are suggesting is basically a concept NACK on this PR. If we aren't confident that we will never get a spent coin from GetCoin, then we need to keep the logic to handle spent and FRESH coins.

💬 Sjors 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#discussion_r1723635626)
It felt a lot longer on my 2019 MBP, and I'm a bit worried the valgrind / sanitizer CI would suffer. But haven't done benchmarking.

I think we should have changed this to 144 back when `nMinerConfirmationWindow` was introduced in #7575. Not sure why it wasn't, @sipa?

💬 sipa commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1723636084)
@paplorinc I feel your criticism here is about the naming/expectations/documentation of the existing code, rather than about this PR. Do you feel that addressing that first should be a blocker for this PR?

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723641345)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723595926

> Here's an alternative using [string literal operator template](https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators) which seems to be working correctly for me:

Wow, that looks great! Assuming this works on compilers we need, it looks like a very nice solution that completely avoids problems I was [struggling with](https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2291521586) trying to
...

👍 theStack approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2248543102)
Code-review ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005

💬 sipa 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#discussion_r1723642677)
@Sjors I can only guess, but I assume it's because I didn't feel like making a consensus rule change to regtest at the same time?

💬 Sjors 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#discussion_r1723650836)
I reduced the magic.

💬 sipa commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299321774)
@paplorinc

> when compiling with GCC with -O1

I don't think we care about performance/binary properties when compiling with anything below -O2.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299336432)
> I don't think we care about performance/binary properties when compiling with anything below -O2.

But the final binary seems to be bigger than before - @hodlinator, did you compare the before/after with `-O3`?

💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1723665550)
I won't block this since I'm fine with discussing/addressing any issues in later commits as well.
I also don't yet understand the second order effects of these changes, so treat my comments as curious observations.

So my concern is that in this PR was that now we're tying `coin.IsSpent()` with retrieving values from the cache.
So if my understanding is correct we should either:
* be able to get a coin from the cache and ignore/assert whether it's spent or not - e.g. [`HaveCoin`](https://g
...

💬 Sjors 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#discussion_r1723673380)
That makes sense.

I don't think it's a big deal though. There's two places where `nPowTargetTimespan` is accessed outside of test code:

1. In `CalculateNextWorkRequired` after the early return in regtest due to `fPowNoRetargeting`
2. In `PermittedDifficultyTransition` used by `headerssync.{h,cpp}`, after the early return in regtest due to `fPowAllowMinDifficultyBlocks`
3. In the value of `DifficultyAdjustmentInterval()` which is used:
i. in `ContextualCheckBlockHeader()` for the timew
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1723693079)
> I approach this from a more functional programming perspective, where I want to infer the possible state changes a method can perform based solely on its signature - or at least what it cannot do.

Yes, that's definitely good and desirable, but my point is that when you add `const` to a c++ function parameter that is passed by value (not passed by reference) the `const` **does not become part of the function signature**. It is completely ignored by callers and has no effect on what inputs ca
...

📝 hebasto opened a pull request: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).

The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.

However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...

💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2299421140)
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).

> As a follow-up, a check for the IBT and SHSTK can be added to the contrib/devtools/security-check.py script.

I don't think we should be making a change this invasive and deferring the addition of checks until later.

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2299449603)
> More about FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target.

Perhaps it would be good to mention this in docs?

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1723759297)
In 9d84717c92ed052dea6f78c715833d328835ba79 "fuzz: Add harness for p2p headers sync": I can be wrong but I think that when sending a compact block, the peer will always reject it due to low-work header?