💬 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)
💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2922380999)
Rebased on top of master.
2c855694c795a31f13a473d6a6eb80c5d765e8ed implements allowing single address or descriptor without using an array or json structure as sugested in https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2112898168 and https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2097722076. But makes `rpc_help.py` fail. (I still don't know why)
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2922380999)
Rebased on top of master.
2c855694c795a31f13a473d6a6eb80c5d765e8ed implements allowing single address or descriptor without using an array or json structure as sugested in https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2112898168 and https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2097722076. But makes `rpc_help.py` fail. (I still don't know why)
💬 fanquake commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2922397804)
> Would be nice to explain this with commands to test and observe the difference in behavior. This would make it easier to understand the goals and test them.
Sure. On master (4b1d48a6866b24f0ed027334c6de642fc848d083):
```bash
# flags in C(XX)FLAGS
make -C depends print-linux_CFLAGS
linux_CFLAGS=-pipe -std=c11
make -C depends print-linux_CXXFLAGS
linux_CXXFLAGS=-pipe -std=c++20
```
make -C depends/ -j20 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1
```bash
# base cflags (-pipe -std=c11)
...
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2922397804)
> Would be nice to explain this with commands to test and observe the difference in behavior. This would make it easier to understand the goals and test them.
Sure. On master (4b1d48a6866b24f0ed027334c6de642fc848d083):
```bash
# flags in C(XX)FLAGS
make -C depends print-linux_CFLAGS
linux_CFLAGS=-pipe -std=c11
make -C depends print-linux_CXXFLAGS
linux_CXXFLAGS=-pipe -std=c++20
```
make -C depends/ -j20 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1
```bash
# base cflags (-pipe -std=c11)
...
🤔 TheCharlatan reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2881363308)
Concept ACK
Guix builds (aarch64)
```
eb7c84cea8ea007cba8605b256f176cfc2c4ee0229fd93a80fca0e0d5fe2f71d guix-build-af227a0e7d97/output/aarch64-linux-gnu/SHA256SUMS.part
b41de2faee47dee6947724e0bbe0dfb69fd91f7a1eb571956b03adda51c73f73 guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu-debug.tar.gz
dfff9d1b7989a31dfa762195c65e09b6427d99bf97bc774fcadf6ed86be4ddbc guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu.ta
...
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2881363308)
Concept ACK
Guix builds (aarch64)
```
eb7c84cea8ea007cba8605b256f176cfc2c4ee0229fd93a80fca0e0d5fe2f71d guix-build-af227a0e7d97/output/aarch64-linux-gnu/SHA256SUMS.part
b41de2faee47dee6947724e0bbe0dfb69fd91f7a1eb571956b03adda51c73f73 guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu-debug.tar.gz
dfff9d1b7989a31dfa762195c65e09b6427d99bf97bc774fcadf6ed86be4ddbc guix-build-af227a0e7d97/output/aarch64-linux-gnu/bitcoin-af227a0e7d97-aarch64-linux-gnu.ta
...
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2922429542)
> OpenBSD are not supported yet, the latter due to a fairly trivial upstream issue.
If it's trivial, I'd say you should pull the patch into depends, rather than opting into a different default behaviour for an entire platform?
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2922429542)
> OpenBSD are not supported yet, the latter due to a fairly trivial upstream issue.
If it's trivial, I'd say you should pull the patch into depends, rather than opting into a different default behaviour for an entire platform?