💬 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
...
💬 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_r2115826232)
I think it's unintuitive that this function returns `true` when the operation failed. It seems like the rate limiting can be carved out pretty much as-is into a separate `bool NeedsRateLimiting()` function? There are only 2 callsites of `FormatLogStrAndRateLimit`, and the first one (`StartLogging`) doesn't look like it needs rate limiting because we already have a max buffer size?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115826232)
I think it's unintuitive that this function returns `true` when the operation failed. It seems like the rate limiting can be carved out pretty much as-is into a separate `bool NeedsRateLimiting()` function? There are only 2 callsites of `FormatLogStrAndRateLimit`, and the first one (`StartLogging`) doesn't look like it needs rate limiting because we already have a max buffer size?
💬 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_r2115869345)
nit: It would be nice if we could catch this in debug builds (ideally even at a fraction, say 20% of the limit), to help inform if our default limit needs to change. `Assume()` is an option, but that would probably break unit tests in debug builds.
nit, because 1MB is small enough that it should prevent issues even on devices with little disk size (unless an attacker finds multiple log vulnerabilities), and large enough that no single unconditional logging statement should ever hit that in t
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115869345)
nit: It would be nice if we could catch this in debug builds (ideally even at a fraction, say 20% of the limit), to help inform if our default limit needs to change. `Assume()` is an option, but that would probably break unit tests in debug builds.
nit, because 1MB is small enough that it should prevent issues even on devices with little disk size (unless an attacker finds multiple log vulnerabilities), and large enough that no single unconditional logging statement should ever hit that in t
...
💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115877367)
@dergoegge might remember why this was added?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115877367)
@dergoegge might remember why this was added?
💬 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_r2115895206)
fwiw: I can't find any discussion about it in https://github.com/bitcoin/bitcoin/pull/21603, and the option was present from the first version of the PR (https://github.com/bitcoin/bitcoin/commit/4648b6d207139ec0ab2994f56c0a47f81cdf516a#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69d)
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115895206)
fwiw: I can't find any discussion about it in https://github.com/bitcoin/bitcoin/pull/21603, and the option was present from the first version of the PR (https://github.com/bitcoin/bitcoin/commit/4648b6d207139ec0ab2994f56c0a47f81cdf516a#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69d)