💬 TheCharlatan commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2921899092)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2921899092)
Concept ACK
🚀 fanquake merged a pull request: "wallet, rpc, gui: List legacy wallets with a message about migration"
(https://github.com/bitcoin/bitcoin/pull/32619)
(https://github.com/bitcoin/bitcoin/pull/32619)
🤔 stickies-v reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2880741415)
Concept ACK on locking down this attack vector, and the code changes required seem acceptable. Also nice to start using `std::source_location` here.
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2880741415)
Concept ACK on locking down this attack vector, and the code changes required seem acceptable. Also nice to start using `std::source_location` here.
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115514944)
nit: I think a brief docstring on their suggested usage here would be helpful here
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115514944)
nit: I think a brief docstring on their suggested usage here would be helpful here
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115526293)
Is there a good use case for disabling rate limiting? I can't think of one. Letting the user define the amount of rate limiting is an alternative, but even then, I'm not sure. If an attacker or bug is able to reach the sane default maximum, then is the user really going to get any benefit from being to inspect the additional volume of un-ratelimited logs? I'm not sure. If so, I think it would make sense to not have this as a configurable option.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115526293)
Is there a good use case for disabling rate limiting? I can't think of one. Letting the user define the amount of rate limiting is an alternative, but even then, I'm not sure. If an attacker or bug is able to reach the sane default maximum, then is the user really going to get any benefit from being to inspect the additional volume of un-ratelimited logs? I'm not sure. If so, I think it would make sense to not have this as a configurable option.
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115570067)
Hmm, I'm feeling uneasy about using `m_categories`, when rate limiting flags fundamentally don't represent a category. I can see how it reduces the diff, but I'm not sure that's worth it. I'll think about an alternative approach - have you considered any already?
Since the user doesn't have to be able to configure any of this, and we only have a single `UNCONDITIONAL_ALWAYS` use case (and likely won't have (many) more in the future), I think adding an `is_conditional=True` arg to `LogPrintLev
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115570067)
Hmm, I'm feeling uneasy about using `m_categories`, when rate limiting flags fundamentally don't represent a category. I can see how it reduces the diff, but I'm not sure that's worth it. I'll think about an alternative approach - have you considered any already?
Since the user doesn't have to be able to configure any of this, and we only have a single `UNCONDITIONAL_ALWAYS` use case (and likely won't have (many) more in the future), I think adding an `is_conditional=True` arg to `LogPrintLev
...
👍 Sjors approved a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2880884812)
utACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2880884812)
utACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
🚀 fanquake merged a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598)
(https://github.com/bitcoin/bitcoin/pull/32598)
💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2922025698)
> The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don't have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?
@hebasto can you take a look here? I ran `test_bitcoin.exe` from this branch (rebased) under wine and didn't see any failures.
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2922025698)
> The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don't have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?
@hebasto can you take a look here? I ran `test_bitcoin.exe` from this branch (rebased) under wine and didn't see any failures.
💬 fanquake commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2922075703)
@ryanofsky want to rebase here?
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2922075703)
@ryanofsky want to rebase here?
🤔 ismaelsadeeq reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2881033332)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2881033332)
Concept ACK
💬 furszy commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2115721183)
> I think empty_local_wallet is unnecessary and the condition can check local_wallet->GetAllScriptPubKeyMans().empty() directly.
That would also delete blank legacy wallets.
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2115721183)
> I think empty_local_wallet is unnecessary and the condition can check local_wallet->GetAllScriptPubKeyMans().empty() directly.
That would also delete blank legacy wallets.
💬 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_r2115756875)
That's a good point, I can't think of any concrete use case to disable rate limiting here.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115756875)
That's a good point, I can't think of any concrete use case to disable rate limiting here.
💬 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_r2115778728)
I haven't considered alternative approaches, but I think adding a boolean is better than adding special-cased categories.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115778728)
I haven't considered alternative approaches, but I think adding a boolean is better than adding special-cased categories.
💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2115800834)
ha.. true. It seems I'm not putting much brain power here.. updated per feedback. Thanks.
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2115800834)
ha.. true. It seems I'm not putting much brain power here.. updated per feedback. Thanks.
👍 real-or-random approved a pull request: "test: update BIP340 test vectors and implementation (variable-length messages)"
(https://github.com/bitcoin/bitcoin/pull/32642#pullrequestreview-2881144268)
utACK b184f5c87c418ea49429e4ce0520c655d458306b
In the long run, it may make sense to depend on (and perhaps vendor) https://github.com/secp256k1lab/secp256k1lab for the EC parts of the test framework. It's essentially an improved version of @sipa's EC test framework code.
(https://github.com/bitcoin/bitcoin/pull/32642#pullrequestreview-2881144268)
utACK b184f5c87c418ea49429e4ce0520c655d458306b
In the long run, it may make sense to depend on (and perhaps vendor) https://github.com/secp256k1lab/secp256k1lab for the EC parts of the test framework. It's essentially an improved version of @sipa's EC test framework code.
💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2115811246)
Done in 4ff4e0194ba6c850d53bba4e281fca5e69386228
The change causes an error in the `rpc_help.py` functional test that I am unable to fix
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2115811246)
Done in 4ff4e0194ba6c850d53bba4e281fca5e69386228
The change causes an error in the `rpc_help.py` functional test that I am unable to fix
💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2115814218)
done thx
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2115814218)
done thx
💬 hebasto commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2922246861)
> > The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don't have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?
>
> @hebasto can you take a look here? I ran `test_bitcoin.exe` from this branch (rebased) under wine and didn't see any failures.
It seems to be related to MSVCRT behaviour, as binaries [linked to UCRT](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15345
...
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2922246861)
> > The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don't have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?
>
> @hebasto can you take a look here? I ran `test_bitcoin.exe` from this branch (rebased) under wine and didn't see any failures.
It seems to be related to MSVCRT behaviour, as binaries [linked to UCRT](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15345
...
💬 vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2922285775)
Yes. That is because if it is accepting incoming connections (has a permanent listening I2P address) then there is just one I2P session which is associated with that address and that session is used for all incoming and outgoing connections.
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2922285775)
Yes. That is because if it is accepting incoming connections (has a permanent listening I2P address) then there is just one I2P session which is associated with that address and that session is used for all incoming and outgoing connections.