💬 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

💬 theuni commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2578280793)
> Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsection
...

💬 theuni commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2578290759)
Not opposed to this, but it seems like a good use-case for a kernel util :)

@thecharlatan @stickies-v @josibake

🤔 glozow reviewed a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2537832865)
reACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7

🚀 glozow merged a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391)