👍 theuni approved a pull request: "build, test: Build `db_tests.cpp` regardless of `USE_BDB`"
(https://github.com/bitcoin/bitcoin/pull/31617#pullrequestreview-2537567612)
utACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501

👍 theuni approved a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616#pullrequestreview-2537578194)
utACK 60b7bbd3709519c6f2f8023a23ee0a194a5f0eb4

💬 maflcko commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2578078036)
Yes, it is a bit fuzzy, but printing the flags to the CI log doesn't solve the problems listed in the motivation either, because to confirm that a flag is used for *all* compilations in a successful compilation, one will have to go through *all* lines. Without a script, I don't see anyone doing that manually. In fact, this change makes the log so bloated that the scrollback will normally truncate it, so it is more likely that the change is going to hide issues instead of surface them.

👍 ryanofsky approved a pull request: "refactor: cache block[undo] serialized size for consecutive calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2537328922)
Code review ACK 2ff0ea366c61b2fcb80ad1f711480915c7a7aa2e. New version of this PR is different and I think easier to follow than the previous version. All the changes now seem like obvious code cleanups.

Might consider renaming PR from "cache block[undo] serialized size" because I'm not sure it's even accurate to call not pointlessly recomputing the same values twice in a row "caching".

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907299844)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (https://github.com/bitcoin/bitcoin/commit/6e22e55920da33dc8970793f9e6a854eb3faa3c4)

IMO in new code it would be better to avoid using `uint32_t` and `unsigned int` for sizes and just use `size_t` for consistency and simplicity. static_casts like this could then be avoided except when calling older functions, and could eventually be removed when older functions are updated.

However, if we do want to keep using n
...

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907341326)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (https://github.com/bitcoin/bitcoin/commit/6e22e55920da33dc8970793f9e6a854eb3faa3c4)

I don't see a reason to delete the comments from this function. I think they are helpful and it is inconsistent to get rid of these because this commit is not deleting the corresponding comments in the WriteUndoDataForBlock function.

I particularly think the "Write index header" comments in both functions are helpful because the
...

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907288195)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (6e22e55920da33dc8970793f9e6a854eb3faa3c4)

Seems like this could be a static assert

EDIT: but I guess it is removed in the upcoming commit, so doesn't matter too much

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907413267)
In commit "scripted-diff: rename block and undo functions for consistency" (2ff0ea366c61b2fcb80ad1f711480915c7a7aa2e)

Agree with hodlinator that Read/Write or Load/Save would now be better than Read/Save. The reason I suggested Save instead of Write in https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-250956282 is that previously we had a high level function for writing called Save and lower level functions called Write, but now after 9b59f8b624cc641bd7216ececffa3111041fd760, th
...

💬 ryanofsky commented on pull request "refactor: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1907322326)
In commit "refactor,blocks: cache block serialized size for consecutive calls" (https://github.com/bitcoin/bitcoin/commit/6e22e55920da33dc8970793f9e6a854eb3faa3c4)

Same comments here about inappropriate use of uint32_t. IMO, we should prefer `size_t` if trying to modernize code or `unsigned int` if trying to be backwards compatible. If there is really a reason for introducing a third size type, it should be stated and clarified somewhere.

💬 maflcko commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#issuecomment-2578081538)
Is there any value in the duplicate timestamp? Seems better to remove it, as per https://github.com/bitcoin/bitcoin/pull/31616#discussion_r1905705466

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2578083931)
> Is this worth it, when the real end-to-end speedup is less than a percent, according to https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900164051?

I think it is useful to have 1749aef52af8ea5f436e5b26dc7281fce73d1436
We can use it in places like bf5c569898d0297de010102a623bf52009607ed8 and maybe more like @l0rinc mentioned https://github.com/bitcoin/bitcoin/pull/31179#pullrequestreview-2526455183.


> Edit: I see you had an end-to-end benchmark in the initial description, bu
...

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907458970)
My concern isn't with the description (I think reserving space can be a valuable change), but that it doesn't really help the user calling this RPC, right?

💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2578115905)
> From my tests, it's a wallet issue (inconsistent state), that only the GUI triggers, The wallet has an invalid state, but the "happy path" in the CLI version doesn't notice it

I agree, my understanding is this: There is a temporary inconsistent state in the wallet when during a rescan the node receives additional blocks (could be via RPC or p2p), that are prematurely processed without updating `m_last_block_processed`. This will self-correct when the `blockConnected` signals for the added b
...

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907471303)
Yes end to end bench it's not significant.
I see it locally as well.

💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2578119305)
> The key isn't used, so deleting it can't hurt backups, and with or without encryption key no ckey records can be added later. Is that correct?

That's correct. ckey is a private key record, and wallets without private keys definitionally do not have private keys. No private keys can be generated or imported to such wallets.

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907474207)
I would be ok with it if we make the benchmarks representative - or not claim that this improved the RPC (I already have the follow-up PR locally using reserve for all other cases, I just want to be honest about it not being a measurable optimization)

💬 achow101 commented on pull request "gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs":
(https://github.com/bitcoin-core/gui/pull/850#issuecomment-2578126891)
> > This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
>
> Two bytes, isn't it? For the PSBT_IN_SIGHASH keytype

No, that field is not added. This is referring to the actual signature that will show up in the scriptWitness, and as appears in `PSBT_IN_TAPROOT_*_SIG` fields beforehand.

👍 theuni approved a pull request: "test: expect that files may disappear from /proc/PID/fd/"
(https://github.com/bitcoin/bitcoin/pull/31614#pullrequestreview-2537652716)
utACK b2e9fdc00f5c40c241a37739f7b73b74c2181e39

I looked around for a way to iterate on a cached list using `os.scandir`, but one doesn't seems to exist. So this makes sense to me.

👍 rkrux approved a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2537728925)
tACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7

Thank you for incorporating the changes.

💬 theuni commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2578207631)
> Wouldn't it be better to add the missing includes? Next to just removing bdb, of course.

Yeah, it seems that it doesn't know `strsep`'s function signature, so it assumes it returns an int. Not sure why this is a new failure though, I guess it's a new warning with gcc14?

It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined. But I'm not sure who's supposed to define that? I suspect that adding it to our cppflags *might* be the
...