💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655273411)
Will add, thanks!
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655273411)
Will add, thanks!
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655277307)
i don't disagree with doing this, but `netif.h` is only included in two places: `net.cpp` and `pcp.cpp`, both of which use `netaddress.h` anyway. Not sure it's worth adding a change in `netaddress.h` in this PR for.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655277307)
i don't disagree with doing this, but `netif.h` is only included in two places: `net.cpp` and `pcp.cpp`, both of which use `netaddress.h` anyway. Not sure it's worth adding a change in `netaddress.h` in this PR for.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655286366)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655286366)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655279201)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
Since #29034, the preferred convention is to use `platform.system()` rather than `os.name`.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655279201)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
Since #29034, the preferred convention is to use `platform.system()` rather than `os.name`.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655288219)
In 38af55e285336cdd3f42635938f24622451703b0 "util: add PermsToString and StringToPerms"
nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655288219)
In 38af55e285336cdd3f42635938f24622451703b0 "util: add PermsToString and StringToPerms"
nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655290944)
In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 "init: add option for rpccookie permissions"
This comment is removed, but the umask is still what sets the permissions in the default case.
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.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655290944)
In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 "init: add option for rpccookie permissions"
This comment is removed, but the umask is still what sets the permissions in the default case.
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.
📝 ryanofsky opened a pull request: "kernel, logging: Pass Logger instances to kernel objects"
(https://github.com/bitcoin/bitcoin/pull/30342)
Pass `Logger` instances to `BlockManager`, `CCoinsViewDB`, `CDBWrapper`, `Chainstate`, `ChainstateManager`, `CoinsViews`, and `CTxMemPool` classes so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.
This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / Lo
...
(https://github.com/bitcoin/bitcoin/pull/30342)
Pass `Logger` instances to `BlockManager`, `CCoinsViewDB`, `CDBWrapper`, `Chainstate`, `ChainstateManager`, `CoinsViews`, and `CTxMemPool` classes so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.
This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / Lo
...
💬 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.
(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
...
(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.
(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
...
(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);`
(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.
(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
(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.
(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
...
(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
(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)
(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
(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`.
(https://github.com/bitcoin/bitcoin/pull/30344#discussion_r1655408815)
Nit: These should be below `node::DEFAULT_STOPATHEIGHT`.