💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655304597)
Hadn't thought of the doc-generation aspect and leaks, good point.

📝 ryanofsky opened a pull request: "wallet, logging: Replace WalletLogPrintf() with LogInfo()"
(https://github.com/bitcoin/bitcoin/pull/30343)
Make new logging macros `LogDebug()`, `LogTrace()`, `LogInfo()`, `LogWarning()`, and `LogError()` compatible with wallet code and drop custom WalletLogPrintf() function. The new logging macros introduced in #28318 weren't previously useful in wallet code because wallet code prefixes most log messages with the wallet names to be be able to distinguish log output from multiple wallets. Fix this by introducing a new `WalletLogSource` class inheriting from `BCLog::Source` which can include the walle
...

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655317117)
Just realized that in the console output in my latest test https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2142445372, the RPC `--user` is hardcoded to `myusername`:
```
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": [/home/hodlinator/.bitcoin/specialWallet/]}'
```
So might as well use the same name if we want to go back to hardcoded platform-specific paths.

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2192368771)
With the latest push, this PR is now much smaller, and now mostly consists of documentation and test changes.

I wanted to split this up in order to be able to base #30342 on a smaller changeset. The wallet changes that were previously part of this PR were moved to #30343.

Rebased ce0004e42d37786f98cdf9d55b976b935dcf1ba1 -> 5f64eab013a58967af37a59c863c2ab6d45ba6e8 ([`pr/bclog.16`](https://github.com/ryanofsky/bitcoin/commits/pr/bclog.16) -> [`pr/bclog.17`](https://github.com/ryanofsky/bitco
...

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