💬 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.
💬 fanquake commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2115856094)
In d64c7fd8fb82e6f2d0bbf3e88b079446119e9ea5:
> but the workaround can be kept, since it makes `has_nx` checks stricter by enforcing both heap and stack are non-executable.
If we do this, can you add a note, for why MACHO has a different check (given `has_nx` exists for it). It could also be worth seeing if upstream would just combine the `has_nx_heap` check into `has_nx` for MACHO?
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2115856094)
In d64c7fd8fb82e6f2d0bbf3e88b079446119e9ea5:
> but the workaround can be kept, since it makes `has_nx` checks stricter by enforcing both heap and stack are non-executable.
If we do this, can you add a note, for why MACHO has a different check (given `has_nx` exists for it). It could also be worth seeing if upstream would just combine the `has_nx_heap` check into `has_nx` for MACHO?
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922301000)
`7e42c92d2b...582d9e3dbd`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922301000)
`7e42c92d2b...582d9e3dbd`: rebase due to conflicts
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922309021)
I believe all issues here have been addressed and this is ready for review. Has a stale ACK from @brunoerg and @jonatack and Concept ACK from @dergoegge.
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2922309021)
I believe all issues here have been addressed and this is ready for review. Has a stale ACK from @brunoerg and @jonatack and Concept ACK from @dergoegge.
💬 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_r2115841360)
Not a big issue, but since we have quite a few log source locations, this class will be instantiated a fair amount of (`N`) times. An alternative approach would be to have a single `LogRateLimiter` instance that encapsulates most of the logic in 6b54362c19ca9baf9dfa9be9281f6faeda169420, and has a single `m_last_reset` member, spawning a minimal `SourceLocationCounter` struct for each source location, containing just the `m_available_bytes` and `m_dropped_bytes` members.
This:
- reduces `N` `
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115841360)
Not a big issue, but since we have quite a few log source locations, this class will be instantiated a fair amount of (`N`) times. An alternative approach would be to have a single `LogRateLimiter` instance that encapsulates most of the logic in 6b54362c19ca9baf9dfa9be9281f6faeda169420, and has a single `m_last_reset` member, spawning a minimal `SourceLocationCounter` struct for each source location, containing just the `m_available_bytes` and `m_dropped_bytes` members.
This:
- reduces `N` `
...
💬 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_r2115859316)
Or perhaps even better than a boolean is a `uint64_t rate_limit=DEFAULT_RATE_LIMIT` so we can do away with the distinction between conditional and unconditional logging entirely? We could use rate_limit=0 as a synonym for unconditional logging, but I think even `UpdateTipLog()` doesn't have to be exempt from rate limiting, we could still set a sane value that allows doing entire IBD in one hour, for example.
Then `UpdateTipLog()` could just use:
```
LogInfo(/*rate_limit=*/100, "%s%s: ne
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115859316)
Or perhaps even better than a boolean is a `uint64_t rate_limit=DEFAULT_RATE_LIMIT` so we can do away with the distinction between conditional and unconditional logging entirely? We could use rate_limit=0 as a synonym for unconditional logging, but I think even `UpdateTipLog()` doesn't have to be exempt from rate limiting, we could still set a sane value that allows doing entire IBD in one hour, for example.
Then `UpdateTipLog()` could just use:
```
LogInfo(/*rate_limit=*/100, "%s%s: ne
...