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

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907547564)
I'm more concerned how the shift will be cast. E.g. a potential future bump to 3000 MiB would error:
```
error: constant expression evaluates to -1149239296 which cannot be narrowed to type 'size_t' (aka 'unsigned long') [-Wc++11-narrowing]
16 | static constexpr size_t DEFAULT_KERNEL_CACHE{3000 << 20};
```
so to make this future proof, I think a function interpreting the values properly, or more ugly casts are required.

💬 theuni commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2578232458)
Also, it seems these were masked by the fact that we use `-Wno-error=implicit-function-declaration -Wno-error=implicit-int` for depends builds.

This is because `exit(0)` which is called a bunch during configure (and there are probably a bunch of others too, but exit is the first that causes problems) is implicitly defined because `stdlib.h` isn't included. Blah.

If this were an important library I'd say we should patch up their configure and remove these warning-off-switches. But since we'
...

👍 ryanofsky approved a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2537795237)
Code review ACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.

👍 theuni approved a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2537798676)
ACK fa029a78780fd00e1d3ce1bebb81a95857bfbb94