💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#discussion_r1655343162)
re: https://github.com/bitcoin/bitcoin/pull/30338#discussion_r1654817299

> This makes the call sites even more verbose and `LogInstance()`, is repeated everywhere

See #30342 for an alternative which should make everything less verbose. For example, this line changes to `LogDebug(m_log, "%s", base);`

📝 theuni opened a pull request: "kernel: remove mempool_persist"
(https://github.com/bitcoin/bitcoin/pull/30344)
DumpMempool/LoadMempool are not necessary for the kernel.

Noticed while working on instantiated logging.

I suppose these could have been left in on purpose, but I'm assuming it was probably just an oversight.

💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2192441080)
ping @TheCharlatan

💬 TheCharlatan commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2192453113)
I've got this on a branch that I'll PR as soon as #30141 is merged: https://github.com/TheCharlatan/bitcoin/commits/kernelRmGlobals/ . I try to keep the open PRs limited to get more focused review.

💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655400466)
re: https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655290944

> This comment is removed, but the umask is still what sets the permissions in the default case.

Good catch, it would be good to add back the comment.

> Since we can set the permissions explicitly in this function, I think it would make sense to always set them with `owner` as default rather than relying on the umask that happens somewhere else.

The PR was actually doing this originally, but I suggested doing th
...

💬 achow101 commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2192463440)
ACK 7d3662fbe35032178c5a5e27e73c592268f6e41b

🚀 achow101 merged a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833)

👍 TheCharlatan approved a pull request: "kernel: remove mempool_persist"
(https://github.com/bitcoin/bitcoin/pull/30344#pullrequestreview-2142765662)
ACK 4d2d9ab66db29a575cf2fab709c2d15044174c7e

💬 TheCharlatan commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#discussion_r1655408815)
Nit: These should be below `node::DEFAULT_STOPATHEIGHT`.

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655351745)
ba75010289f1d9bd3d3a6c635ad18b07cf914041

This is copy/paste the comment for `NumToOpenSub()` but doesn't apply to this get-only method.

Also, the comments for the two following functions are missing `@param[in] n ...` but I don't know how strict you need to be about doxygen comments for everything.

🤔 pinheadmz reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2142672720)
Re-reviewed first half of commits, will continue tomorrow. A few comments and questions below.

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655423362)
b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Why isn't this at the top of the super long function (and include `VERACK` also?)

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655402373)
cade8e833197a202dbb03c50359396ecd437ac8c

optional, could use a comment explaining the priority metric. Looks like transactions with fewer broadcasts or older broadcasts are prioritized. A unit test would help make that clear as well.

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655358621)
ba75010289f1d9bd3d3a6c635ad18b07cf914041

Why pass this in here instead of just checking it before calling?

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655433726)
b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Isn't this `GetTime()` deprecated?

https://github.com/bitcoin/bitcoin/blob/7a7e7d189dfe5efa6ab3c644f55f410412163503/src/util/time.h#L95-L100

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655427794)
b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Is it ok to erase from this map now? What if none of our connections are successful ?

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1655435296)
In f732495191d346b8:

The reset call needs to be moved above the `reload_wallet` function declaration. Otherwise, the wallet will be reloaded in read-only mode for any failure occurring in-between the `reload_wallet` declaration and this line, such as failures during backup creation or the decryption procedure.

I have written a test to trigger this misbehavior: furszy@c24b673 (feel free to pull it here). The test passes when the fix is cherry-picked on top: furszy@2d5d9a4.

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2192524319)
Rebased to remove a workaround that was added in the meantime.

🤔 furszy reviewed a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2142868185)
ACK functionality wise. Not blocking if others are happy with the current code.

Have to admit that I'm not fan of the multiple `CTxOut` creations and `GetScriptForDestination` calls when we could cache the `CTxOut` early in the process, leaving the silent payment destinations empty, just so they can be modified later on. The first commit `GetSerializeSizeForRecipient` and `IsDust` decouplings seem more like an unneeded indirection rather than something worth doing to me atm. But as said above
...

💬 TheCharlatan commented on pull request "kernel, logging: Pass Logger instances to kernel objects":
(https://github.com/bitcoin/bitcoin/pull/30342#issuecomment-2192559502)
Concept ACK.