💬 brunoerg commented on issue "`keypoolrefill` doesn't fill keypool to specified parameter":
(https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2098939485)
I think you have more than one `ScriptPubKeyManager`. When you call `keypoolrefill ` it will top up every spkm. Did you check `listdescriptors`?

💬 luke-jr commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2098941527)
Is there a benefit to this? Just dropping patches?

💬 luke-jr commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2098953741)
Is it possible to use a UTS namespace to spoof the clock just for the build?

💬 luke-jr commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2098989028)
If there's no drawbacks, why not go even larger?

💬 hebasto commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2098990473)
> > Wait for 24.04 GHA image, Move the task over to that image
>
> Still waiting for it to appear here: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

The wait is over: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20240430.1

💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592872178)
This code snippet doesn't work for me on Sage 9.5 (see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/292). Also, we only need to output the x-coordinate here:
```suggestion
G2 = EllipticCurve ([F (0), F (7)]).lift_x(F(int(hashlib.sha256(bytes.fromhex(G_DER)).hexdigest(),16)))
print('%x' % G2.xy()[0])
```
(That said, I think it's also completely fine to not include any sage code here and instead just keep the first paragraph.)

💬 luke-jr commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2099010611)
Why?

👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2041313158)
Code review ACK 194e84accced947ef63c6db389bc62a2b58cffa3. I left a lot of comments, but everything looks right here and the code is a lot nicer than before.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591338361)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)

It looks like previously there would have been an error here if `dbp->IsNull()` was true, and now there will not be an error. This is probably a good change, since AcceptBlock should not be looking at block positions, just passing them on.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591463595)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)

This comment seems really disconnected from the statement below it, because the statement does not mention reindexing or the snapshot chainstate at all. I left a suggestion to improve the comment below.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591477885)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

Would be helpful to clarify with meaning of `true` with `/*fFinalize*/=true`

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591461829)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)

I don't think changing this `if` statement is good.

Adding the Assume call above seems good, since it provides information about the context this code is called in and could potentially catch bugs if the code is run in an unanticipated state.

But It's less clear what benefit there is to adding the `!fKnown &&` condition to this if statement. It just makes the logic more comp
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592739006)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think this description is technically accurate, but I got confused by it and thought it was wrong because "the 8 byte serialization header" sounds like something that is part of `CBlock` serialization, when actually it is referring to separator fields written by `WriteBlockToDisk` *before* the serialized `CBlock`.

Would suggest changing comment to "pos: the position of the serialized CBloc
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592707738)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

Two suggestions, maybe for later commits or a followup PR:

Now that `pos` is an output parameter instead of being an in/out parameter, it would be better to just drop it entirely and make `FindBlockPos` return `FlatFilePos` like `SaveBlockToDisk`, instead of returning `bool`. This would make it more obvious what the function inputs and outputs are, and also make sure the output value is cons
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592833124)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think `SaveBlockPos` would be a less ambiguous name for this function than `AddToBlockFileInfo`. It would also be consistent with `FindBlockPos` and I think make the `AcceptBlock` code more obvious (`if (dbp) SaveBlockPos(...) else SaveBlockToDisk(...)`)

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592824870)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think this comment is a little confusing, because it isn't obvious that `position_known` can only be true during reindexing. Could potentially clarify this, though not necessary since this code will be deleted in the next commit.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592848815)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)

s/do/to/

💬 achow101 commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2099011321)
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3

🚀 achow101 merged a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494)

💬 luke-jr commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2099035706)
@achow101 Are you sure you want to put this on a common domain with other things?