💬 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".

💬 GregTonoski commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2299620718)
> 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".

Why not automatically unloading a wallet if it prevents bitcoind from completing start-up?

💬 fjahr commented on issue "wallet: lastprocessedblock is be inconsistent with internal best block":
(https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299634338)
> Another more minimalistic approach could be to show users the relevant block locator. That may be in getwalletinfo() or the return value of backupwallet().

Here is rough draft of what that could look like. I am not if that is a good solution as mentioned above but if people think it's valuable I can clean it up and open a PR: https://github.com/fjahr/bitcoin/commit/fa4c9978a50802ad3ada5b6dce370f57b8ec16b8

💬 fjahr commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299635058)
> It would be great if we could open an issue describing the user behavior that's confusing.

I have created an issue here: #30686

💬 ryanofsky commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#issuecomment-2299641553)
Code review 9f19d10922d615c1e313eb7348fcc536642ad2e4.

I'm not sure this fix is ideal. For example if the GUI is used I believe this will change the error message from:

- Error: Invalid port specified in -rpcbind: '127.0.0.1:notaport'

to just:

- Unable to start HTTP server. See debug log for details.

I think it would be good to keep the new `return false` and error log in `HTTPBindAddresses`. But a more direct fix for this issue might be to just move the code that checks port numbe
...

💬 ryanofsky commented on issue "wallet: lastprocessedblock is be inconsistent with internal best block":
(https://github.com/bitcoin/bitcoin/issues/30686#issuecomment-2299674756)
Thanks for explaining all this.

fa4c9978a50802ad3ada5b6dce370f57b8ec16b8 could solve the immediate problem if users notice the return value, but it doesn't seem as robust a solution as updating the wallet before creating the backup like 4ffdc9b70711b2bec0e6e86a4f2cc0a87de406d2 is trying to do. Only problem with that commit is it might not be writing the right locator:

```c++
if (m_chain) {
WalletBatch batch{GetDatabase()};
CBlockLocator loc{m_chain->getTipLocator()};
...

👍 brunoerg approved a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2249000981)
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2