🤔 ryanofsky reviewed a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2028926937)
Code review 697d0a23f5d0a75829ffe1ac874eaf15fff3bfbe. Approach seems good and first commit seems ok in its current form but I think version of GetPruneHeight in the second commit is not handling the genesis block correctly.

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583454227)
In commit "rpc: Make pruneheight also reflect undo data presence" (61f352d6bcf485862815fdd68064cc07ea8ab6ce)

This doesn't seem right. If chain consists of just genesis block, and genesis block doesn't have undo data this will return height 0 as if genesis block were pruned.

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583398465)
In commit "refactor, blockstorage: Generalize GetFirstStoredBlock" (4e62129d8aed839c83fa4a595aab06ed5ca7eeb6)

IMO, this logic is a little confusing because it using both `nHeight` and `pprev` instead of just using `pprev` consistently. Also I think the cases here could have more explanation.

```c++
// Get first block with data, after the last block without data.
// This is the start of the unpruned range of blocks.
const auto first_unpruned{active_chainstate.m_blockman.GetFirstBlock(cha
...

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583472496)
In commit "refactor, blockstorage: Generalize GetFirstStoredBlock" (4e62129d8aed839c83fa4a595aab06ed5ca7eeb6)

I don't think it makes sense to call this parameter `active_chainstate`. It is true the active chainstate is always passed, but this function should care whether the chainstate is active or not. Probably also it would be better for this function not to access the Chainstate class at all and instead be passed separate BlockMan and CChain parameters.

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583515582)
In commit "rpc: Make pruneheight also reflect undo data presence" (61f352d6bcf485862815fdd68064cc07ea8ab6ce)

This comment "Result can never be 0 because genesis block is never pruned" is misleading and basically describes the opposite of what is happening. If the genesis block was not pruned you would expect GetFirstBlock to return the genesis block, because it has data, and for the result to still be 0 here like it was in the previous commit. The reason `GetFirstBlock` is no longer returning
...

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583335298)
In commit "refactor, blockstorage: Generalize GetFirstStoredBlock" (dd7696b97f20d756df1afa523a8f9e7bf3924c7d)

I think there are a few issues with this comment:

- The @<!-- -->brief and @<!-- -->return sections make it sound like it is returning the earliest block matching the flags, when actually it is returning the earliest block matching the flags after the latest block _not_ matching them. This is different because it means not only does the returned block have the flags, but all blocks
...

📝 Sjors opened a pull request: "guix: fix suggested fake date for openssl-1.1.1l"
(https://github.com/bitcoin/bitcoin/pull/29999)
Using `2020-10-01` as the fake timestamp will cause many test failures with `/gnu/store/bfirgq65ndhf63nn4q6vlkbha9zd931q-openssl-1.1.1l.drv`. I didn't investigate why, but I guess because it's _before_ the test certificates were created. They expired in June 2022. I tried a month before that, which worked.

Also fixes layout of instructions.

🤔 MarnixCroes reviewed a pull request: "gui: fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2029148876)
cack

💬 MarnixCroes commented on pull request "gui: fix misleading signmessage error with segwit":
(https://github.com/bitcoin-core/gui/pull/819#discussion_r1583469857)
- it doesn't work for P2SH either, which doesn't start with bc1/tb1

- I'd probably leave out the testnet/signet example/mentioning of tb1, to prevent confusion, or remove `addresses staring with...` in full

- nit: maybe write segwit with capitals S and W as used in the rest of the UI

💬 Sjors commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2083440214)
cc @dongcarl

💬 AngusP commented on pull request "test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type":
(https://github.com/bitcoin/bitcoin/pull/29748#issuecomment-2083451948)
tACK e233ef18364a5758adcb83eea53bdf92aa1bf551

💬 laanwj commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2083468516)
Changing the system date is a terrible workaround in any case imo. Is there really no other way?

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583635647)
done.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583636377)
Yes, I think so. We later may increase nFile and then compare it to `last_blockfile` (line `if (nFile != last_blockfile)`)

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583637879)
I took both from FindBlockPos, snake case is correct, but for historical reasons camel case is still used in lots of places.
But since this is arguably new code I renamed `nAddSize` to `added_size`

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583638770)
You mean the comment "The ASSUMED state is initialized, when necessary, in FindBlockPos()."?
That behavior is unchanged, it's part of the normal usage of `FindBlockPos` and wasn't happening during reindex anyway (see first commit), so I don't think it needs to be updated.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583638866)
done

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583639069)
done

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583640596)
Yes. I added this in the first commit, so that the second commit doesn't change behavior.

As for your second q: Well, it'd be undefined behavior if `m_blockfile_cursors` didn't haven an element for `BlockfileType::NORMAL`. On the other hand, `m_blockfile_cursors[...]` is used all over the place, I'm not sure if we want to have an assert for each occurence. Other opinions?

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1583642139)
I updated some comments. The entire test setup probably makes a bit less sense after the refactor, users unfamiliar
with the history might ask themselves why Reindex / AddToBlockFileInfo would change the block files
so that we'd require a test making sure it doesn't.