💬 achow101 commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#issuecomment-3029243288)
ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
(https://github.com/bitcoin/bitcoin/pull/32580#issuecomment-3029243288)
ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
🚀 achow101 merged a pull request: "test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()"
(https://github.com/bitcoin/bitcoin/pull/32823)
(https://github.com/bitcoin/bitcoin/pull/32823)
💬 achow101 commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3029320429)
ACK 0ab0e1be5461084b8e945f14406868f044c90718
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3029320429)
ACK 0ab0e1be5461084b8e945f14406868f044c90718
💬 glozow commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180971720)
Yes, those shouldn't be included!
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180971720)
Yes, those shouldn't be included!
💬 tnndbtc commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3029345008)
@GregTonoski The tests I have done is to prove that
1) making debug=1 have turned on too much logs and it clearly slows down the IBD process even in a fast device (SSD).
2) levelDB can be tuned to have less disk operations while each time bitcoind can flush more data from memory to disk, if disk I/O is not sufficiently leveraged.
I agree that for a fair performed hardware, bitcoind shouldn't take too long to sync. So, I googled average write speed of HDD, the first entry (generated by their A
...
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3029345008)
@GregTonoski The tests I have done is to prove that
1) making debug=1 have turned on too much logs and it clearly slows down the IBD process even in a fast device (SSD).
2) levelDB can be tuned to have less disk operations while each time bitcoind can flush more data from memory to disk, if disk I/O is not sufficiently leveraged.
I agree that for a fair performed hardware, bitcoind shouldn't take too long to sync. So, I googled average write speed of HDD, the first entry (generated by their A
...
💬 glozow commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180989848)
True, there isn't actually anything to group in this test. Just copied from the other tests.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180989848)
True, there isn't actually anything to group in this test. Just copied from the other tests.
💬 glozow commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3029351762)
reACK 1632fc104be via range-diff
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3029351762)
reACK 1632fc104be via range-diff
🤔 glozow reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2980663908)
reACK 1632fc104be via range-diff
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2980663908)
reACK 1632fc104be via range-diff
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181013847)
> Lastly, do we really need the RemovePrefix here, given that we know the exact file structiure?
Yup, the file name may differ across platforms and the test will fail on some platforms otherwise. Necessary because it's done here: https://github.com/bitcoin/bitcoin/blob/68ca13e1f96a9dae997558a1d7a873b10056091b/src/logging.cpp#L377
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181013847)
> Lastly, do we really need the RemovePrefix here, given that we know the exact file structiure?
Yup, the file name may differ across platforms and the test will fail on some platforms otherwise. Necessary because it's done here: https://github.com/bitcoin/bitcoin/blob/68ca13e1f96a9dae997558a1d7a873b10056091b/src/logging.cpp#L377
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181018846)
I have kept the comment -- I made a note in the commit message about the change.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181018846)
I have kept the comment -- I made a note in the commit message about the change.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181022825)
Now uses a parameter, but I have kept the comment. I think it is clear as is? I think a follow-up could address this?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181022825)
Now uses a parameter, but I have kept the comment. I think it is clear as is? I think a follow-up could address this?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181023549)
I haven't implemented this, could this be left for a follow-up?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181023549)
I haven't implemented this, could this be left for a follow-up?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181024709)
I've modified the comment so it doesn't repeat the phrase "rate limit" twice.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181024709)
I've modified the comment so it doesn't repeat the phrase "rate limit" twice.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181026104)
Modified the commit message and have checked that `MemUsage` returns an accurate accounting of the memory used. Thanks for this comment, it encouraged me to verify this.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181026104)
Modified the commit message and have checked that `MemUsage` returns an accurate accounting of the memory used. Thanks for this comment, it encouraged me to verify this.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181027949)
I have not run benchmarks, though this `insert` is pre-existing so will resolve. It would be nicer if there was a better way to format the string other than the insert-at-beginning logic in `FormatLogStrInPlace`.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181027949)
I have not run benchmarks, though this `insert` is pre-existing so will resolve. It would be nicer if there was a better way to format the string other than the insert-at-beginning logic in `FormatLogStrInPlace`.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181028316)
Modified the commit message 👍
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181028316)
Modified the commit message 👍
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029394)
I agree this is confusing and threw me. The different base part is pre-existing so will resolve. I think another PR could eliminate the different bases unless there's a compelling "historical" reason to keep both?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029394)
I agree this is confusing and threw me. The different base part is pre-existing so will resolve. I think another PR could eliminate the different bases unless there's a compelling "historical" reason to keep both?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029843)
Latest change address this.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029843)
Latest change address this.
💬 achow101 commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3029409482)
> which is that timers are mapped by name and can be removed by name if, for example, the user calls the RPC again with a different timeout value.
This behavior would already be changing in this PR. Deleting the `HTTPRPCTimer` does not de-schedule the task and it will still execute later.
> Current behavior I believe is to only honor the most recent walletpassphrase argument.
This behavior would still be maintained even if the task runs multiple times because there is an explicit check
...
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3029409482)
> which is that timers are mapped by name and can be removed by name if, for example, the user calls the RPC again with a different timeout value.
This behavior would already be changing in this PR. Deleting the `HTTPRPCTimer` does not de-schedule the task and it will still execute later.
> Current behavior I believe is to only honor the most recent walletpassphrase argument.
This behavior would still be maintained even if the task runs multiple times because there is an explicit check
...
💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181030545)
I doubt anyone will review it if we push it separately - but I think we could make these tests less awkward. Not a blocker from my side, just a preference
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181030545)
I doubt anyone will review it if we push it separately - but I think we could make these tests less awkward. Not a blocker from my side, just a preference