💬 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?

💬 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#issuecomment-2299500214)
> Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.

I think for a change like this, spending time fuzzing would be more valuable. I believe it already runs in debug mode for that so `Assume`s would trigger crashes.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299511984)
> But the final binary seems to be bigger than before - @hodlinator, did you compare the before/after with -O3?

@paplorinc I compared before/after using a guix-build since that should be representative, don't know which optimization level it has. Updated the relative size increase in the PR summary ~6 hours ago, based on the latest push (c3fa29db1b05aa51f74cc8be5bdae59be4f3b7c0). +0.04% seems okay if you ask me.

⚠️ fjahr opened an issue: "wallet: lastprocessedblock is be inconsistent with internal best block"
(https://github.com/bitcoin/bitcoin/issues/30686)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

`m_last_block_processed` and `m_last_block_processed_height`, which the users can see in their `getwalletinfo()` result as `lastprocessedblock` > `height`, are not guaranteed to match the block locator stored in the wallet. In practice this means that if the user checks this height and then creates a backup of the wallet they might expect that the wallet will only need to continue syncing
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723800889)
That is awesome! Didn't realize I had to *enable* C++20 support on Godbolt. :man_facepalming: Thank you for persisting @paplorinc! Will experiment with this.

💬 furszy commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2299595304)
The error message could suggest an alternative path to follow, such as: "If you want to proceed without using this wallet, use '-disablewallet=<wallet_name>' during startup".