π¬ paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2197292114)
> your time might be better spent reviewing Pull Requests like https://github.com/bitcoin/bitcoin/pull/28280
Thanks for the hint @fanquake, it's a tough PR, I have added my two sats to it: https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2138727793
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2197292114)
> your time might be better spent reviewing Pull Requests like https://github.com/bitcoin/bitcoin/pull/28280
Thanks for the hint @fanquake, it's a tough PR, I have added my two sats to it: https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2138727793
π¬ paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659023545)
I don't fully understand how we need `m_prev` here, but have you investigated whether it's possible to get rid of it and rely on `m_next` only?
Sometimes we still have a reference to both, but I assume that wasn't always the case, right?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659023545)
I don't fully understand how we need `m_prev` here, but have you investigated whether it's possible to get rid of it and rely on `m_next` only?
Sometimes we still have a reference to both, but I assume that wasn't always the case, right?
π¬ ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659025992)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164
> Meta:
>
> It's not clear why people are π on that comment.
I kind of like the mystery of the reactions. But I π AJ's chart because I think it is a good recommendation for how to choose log levels in new code, even if it doesn't really tell you what log levels mean in existing code. I also broadly agree with having few log levels and using few log levels. I think by a large margin most log statements should be u
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659025992)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164
> Meta:
>
> It's not clear why people are π on that comment.
I kind of like the mystery of the reactions. But I π AJ's chart because I think it is a good recommendation for how to choose log levels in new code, even if it doesn't really tell you what log levels mean in existing code. I also broadly agree with having few log levels and using few log levels. I think by a large margin most log statements should be u
...
π¬ jonatack commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197318437)
> All consistent, now accepting the same parameters
> All compatible with log categories, to make it possible to filter or highlight log messages from a particular component
> All compatible with wallet logging, which includes the wallet name in log messages
> Require less verbosity, by not needing category constants to be repeated in log prints from a common source.
> Not tied to one hardcoded global Logger instance
I think I'd be Concept ACK on these, though I've some
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197318437)
> All consistent, now accepting the same parameters
> All compatible with log categories, to make it possible to filter or highlight log messages from a particular component
> All compatible with wallet logging, which includes the wallet name in log messages
> Require less verbosity, by not needing category constants to be repeated in log prints from a common source.
> Not tied to one hardcoded global Logger instance
I think I'd be Concept ACK on these, though I've some
...
π¬ tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2197343758)
> thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
>
> Also @tdb3 I tried doing
>
> ```
> b1hash = index_node.getblockhash(1)
> index_node.invalidateblock(b1hash)
>
> self.log.info("Test obtaining info for a non-existent block hash")
> assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
>
> index_n
...
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2197343758)
> thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
>
> Also @tdb3 I tried doing
>
> ```
> b1hash = index_node.getblockhash(1)
> index_node.invalidateblock(b1hash)
>
> self.log.info("Test obtaining info for a non-existent block hash")
> assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
>
> index_n
...
π¬ ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2197354127)
Concept NACK; introducing more different levels just makes it require more effort to use the logging system, and increases the chances they will be used incorrectly.
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2197354127)
Concept NACK; introducing more different levels just makes it require more effort to use the logging system, and increases the chances they will be used incorrectly.
π¬ ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197361518)
> We've just over-complicated everything when it could be simple and open. I know a number of other developers think the same but don't speak up much because it seems pointless to do so.
Curious, when you say "over-complicated", are you referring to the implementation of the logging code, or to the usage of the logging API? Or to effects of logging settings?
I don't think the logging API is too complicated. I think #28318 made it more self documenting and simpler.
On the other hand, I w
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2197361518)
> We've just over-complicated everything when it could be simple and open. I know a number of other developers think the same but don't speak up much because it seems pointless to do so.
Curious, when you say "over-complicated", are you referring to the implementation of the logging code, or to the usage of the logging API? Or to effects of logging settings?
I don't think the logging API is too complicated. I think #28318 made it more self documenting and simpler.
On the other hand, I w
...
π¬ ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659082202)
> Your [flowchart](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657886163) is a good idealization, but it does not describe the realities of current code, which is using logging for ordinary error reporting, not just the most critical errors.
The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659082202)
> Your [flowchart](https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657886163) is a good idealization, but it does not describe the realities of current code, which is using logging for ordinary error reporting, not just the most critical errors.
The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that
...
π¬ ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893)
It's not clear to me that there's any reason to separate the current `LogWarning` / `LogError` levels; those existed prior to #28318 and I documented them in that PR as best I understood them. However, there's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." immediately following. So it could make sense to merge Warning/Error into:
* - `LogAlert(fmt, params...)` should be used in p
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893)
It's not clear to me that there's any reason to separate the current `LogWarning` / `LogError` levels; those existed prior to #28318 and I documented them in that PR as best I understood them. However, there's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." immediately following. So it could make sense to merge Warning/Error into:
* - `LogAlert(fmt, params...)` should be used in p
...
π theuni approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2148613939)
utACK 064a401e10f2b2da8f31f682929431d06c671d93 assuming sipa doesn't have a problem with the shared nonce.
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2148613939)
utACK 064a401e10f2b2da8f31f682929431d06c671d93 assuming sipa doesn't have a problem with the shared nonce.
π€ pinheadmz reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2148337702)
code review ACK 7a7e7d189dfe5efa6ab3c644f55f410412163503
some questions and comments above and below (sorry spread out review over a few days)
I want to run this in warnet again with a lot more TXs and see how we do as well
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2148337702)
code review ACK 7a7e7d189dfe5efa6ab3c644f55f410412163503
some questions and comments above and below (sorry spread out review over a few days)
I want to run this in warnet again with a lot more TXs and see how we do 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_r1659087156)
0a6797ffb3eb2d45bce1dc68e830efd89ac2a442
Teleporting the comment from https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864
Why didn't we need to do this on master?
The old log message was `Connected & Listening: 127.0.0.1:15689` not `Connected & Listening: 0:0`
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1659087156)
0a6797ffb3eb2d45bce1dc68e830efd89ac2a442
Teleporting the comment from https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1592701864
Why didn't we need to do this on master?
The old log message was `Connected & Listening: 127.0.0.1:15689` not `Connected & Listening: 0:0`
π¬ pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658932157)
a86785dc29faa2dcff301641e2f823b73089dae2
should `1min` be defined as a constant in the header file? This is what we consider stale?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658932157)
a86785dc29faa2dcff301641e2f823b73089dae2
should `1min` be defined as a constant in the header file? This is what we consider stale?
π¬ pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658942976)
5f49e61ffa576aa6c27de30a7db51d17c4f5c516
How could this be condition be different now than when it is checked in init.cpp?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1658942976)
5f49e61ffa576aa6c27de30a7db51d17c4f5c516
How could this be condition be different now than when it is checked in init.cpp?
π¬ jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659120894)
> The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that as a blocker for any other logging changes.
The logging was mostly migrated in the severity level parent PR #25203 before the whole thing was taken off the rails with the overwrought API and rules. (There have also been a number of pulls opened that were
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659120894)
> The current code mostly hasn't migrated to the multiple severity levels from the original LogPrintf/LogPrint split. It would be nice to spend time working on that, but we're instead constantly bikeshedding the API instead and treating that as a blocker for any other logging changes.
The logging was mostly migrated in the severity level parent PR #25203 before the whole thing was taken off the rails with the overwrought API and rules. (There have also been a number of pulls opened that were
...
π hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2148642582)
ACK 69bf58dc0e25897e9fde435c9823a921590a90dc
Built and tested `bitcoin-cli -regtest loadwallet`.
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2148642582)
ACK 69bf58dc0e25897e9fde435c9823a921590a90dc
Built and tested `bitcoin-cli -regtest loadwallet`.
π¬ hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659121673)
Could reduce it down to only one `HelpExampleRpc` if one would want to reduce the number of lines.
```suggestion
+ "\nLoad wallet with files under wallets/foo/ via RPC:\n"
+ HelpExampleRpc("loadwallet", "\"foo\"")
+ "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
+ HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
+ "\nLoad wallet using absolute path (Unix):\n"
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1659121673)
Could reduce it down to only one `HelpExampleRpc` if one would want to reduce the number of lines.
```suggestion
+ "\nLoad wallet with files under wallets/foo/ via RPC:\n"
+ HelpExampleRpc("loadwallet", "\"foo\"")
+ "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
+ HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
+ "\nLoad wallet using absolute path (Unix):\n"
...
π¬ andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2197401102)
> Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.
Thank you @paplorinc! I did try initially to separate the changes more. However, I found the third commit has to have all those changes done in one commit otherwise the test-all-commits CI job will fail. Changing only parts of it can break the tests, and break buildin
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2197401102)
> Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.
Thank you @paplorinc! I did try initially to separate the changes more. However, I found the third commit has to have all those changes done in one commit otherwise the test-all-commits CI job will fail. Changing only parts of it can break the tests, and break buildin
...
π¬ tdb3 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2197420105)
> > How about squashing some of these changes to help keep the master branch commit log cleaner?
>
> Done
Looks like b382bf4755e2b503cb1ee2f87a7b0f66dc5b76f4 adds the if, then d2e86117bc6004515ec72e55b2ec57304a31c0eb removes it. This seems extraneous. Could these commits be squashed?
```c++
if (gArgs.IsArgSet("-getinfo") +
gArgs.GetBoolArg("-netinfo", false) +
gArgs.GetBoolArg("-generate", false) +
gArgs.GetBoolArg("-addrinfo", false) >
...
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2197420105)
> > How about squashing some of these changes to help keep the master branch commit log cleaner?
>
> Done
Looks like b382bf4755e2b503cb1ee2f87a7b0f66dc5b76f4 adds the if, then d2e86117bc6004515ec72e55b2ec57304a31c0eb removes it. This seems extraneous. Could these commits be squashed?
```c++
if (gArgs.IsArgSet("-getinfo") +
gArgs.GetBoolArg("-netinfo", false) +
gArgs.GetBoolArg("-generate", false) +
gArgs.GetBoolArg("-addrinfo", false) >
...
β οΈ djkarmi opened an issue: "Slow sync "
(https://github.com/bitcoin/bitcoin/issues/30360)
Why does my Bitcoin daemon take so long to synchronize?
Rasppery pi 4 8GB
Bitcoin v27 64 bit gnu
bitcoin.conf
#Opcje Bitcoind
server=1
daemon=1
testnet=0
txindex=0
prune=10000
#poΕΔ czenie RPC
rpcuser=xyz
rpcpassword=xyz
zmqpubrawblock=tcp://127.0.0.1:29000
zmqpubrawtx=tcp://127.0.0.1:29001
#Optymalizacja dla maliny
#dbcache=200
#maxorphantx=10
#maxmempool=50
#maxmempool=300
maxconnections=50
maxuploadtarget=5000
$ tail -f ~/.bitcoin/debug.log
2024-06-
...
(https://github.com/bitcoin/bitcoin/issues/30360)
Why does my Bitcoin daemon take so long to synchronize?
Rasppery pi 4 8GB
Bitcoin v27 64 bit gnu
bitcoin.conf
#Opcje Bitcoind
server=1
daemon=1
testnet=0
txindex=0
prune=10000
#poΕΔ czenie RPC
rpcuser=xyz
rpcpassword=xyz
zmqpubrawblock=tcp://127.0.0.1:29000
zmqpubrawtx=tcp://127.0.0.1:29001
#Optymalizacja dla maliny
#dbcache=200
#maxorphantx=10
#maxmempool=50
#maxmempool=300
maxconnections=50
maxuploadtarget=5000
$ tail -f ~/.bitcoin/debug.log
2024-06-
...