💬 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#issuecomment-3045985477)
The latest push f47e2ea9137c3a832e07d6dd845c55d35d533fa9 has a few (minor) changes:
- removing the comment in `logging_filesize_rate_limit`
- documents restart behavior in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
- fixes the typo of `-logsourcelocations` in the release notes
- makes the release notes more clear wrt. `LogPrintLevel`
> We're getting closer, but the PR is not ready yet. I went over the code multiple times, left nits and comments and questions all over,
...
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3045985477)
The latest push f47e2ea9137c3a832e07d6dd845c55d35d533fa9 has a few (minor) changes:
- removing the comment in `logging_filesize_rate_limit`
- documents restart behavior in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
- fixes the typo of `-logsourcelocations` in the release notes
- makes the release notes more clear wrt. `LogPrintLevel`
> We're getting closer, but the PR is not ready yet. I went over the code multiple times, left nits and comments and questions all over,
...
💬 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_r2190472363)
IIRC, logging a 1024-byte line in my test had a memory overhead of ~1140 bytes. Prior to the change it was ~1200 bytes or so.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190472363)
IIRC, logging a 1024-byte line in my test had a memory overhead of ~1140 bytes. Prior to the change it was ~1200 bytes or so.
💬 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_r2190508458)
My mistake, addressed. This might have been a rebase artifact, I was pretty sure that I removed it.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190508458)
My mistake, addressed. This might have been a rebase artifact, I was pretty sure that I removed it.
💬 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_r2190500196)
> On the other hand, is it really necessary to do the accounting per line, aren't we over-complicating a hypothetical scenario?
I don't think this is hypothetical -- security vulnerabilities these days are increasingly multi-layered and complex and it is not unheard of for attackers to chain together exploits. I have personally had to use logs to piece together what happened after-the-fact when somebody's machine was compromised. The concern brought up in the earlier PRs was about an attacker
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190500196)
> On the other hand, is it really necessary to do the accounting per line, aren't we over-complicating a hypothetical scenario?
I don't think this is hypothetical -- security vulnerabilities these days are increasingly multi-layered and complex and it is not unheard of for attackers to chain together exploits. I have personally had to use logs to piece together what happened after-the-fact when somebody's machine was compromised. The concern brought up in the earlier PRs was about an attacker
...
💬 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_r2190596839)
I like comments because it gives you an overview of the code to come. I can understand the aversion to comments, but I personally find them pretty helpful especially here. Sure, the code is the real source of truth, but comments have their use and I think usage here is reasonable.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190596839)
I like comments because it gives you an overview of the code to come. I can understand the aversion to comments, but I personally find them pretty helpful especially here. Sure, the code is the real source of truth, but comments have their use and I think usage here is reasonable.
💬 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_r2190479251)
I think this is safe. I understand the concern, but the `NOLINT` and the comment next to the recursion should be a bright red flag to anybody modifying the code in the future.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190479251)
I think this is safe. I understand the concern, but the `NOLINT` and the comment next to the recursion should be a bright red flag to anybody modifying the code in the future.
💬 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_r2190599408)
Have documented this in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190599408)
Have documented this in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
💬 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_r2190580292)
> Also, this change is meant to avoid an attack, we don't have to be that precise with the calculations, just a rough accounting should suffice as far as I can tell - especially if it makes the code simpler.
The accounting could be less precise, I had not considered that. I like that this PR is precise and I can account for the bytes.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190580292)
> Also, this change is meant to avoid an attack, we don't have to be that precise with the calculations, just a rough accounting should suffice as far as I can tell - especially if it makes the code simpler.
The accounting could be less precise, I had not considered that. I like that this PR is precise and I can account for the bytes.
💬 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_r2190506106)
> But shouldn't we only prefix the dropped lines by * anyway (why prefix the lines that aren't dropped in the first place?), so why do we care about a global suppression state, i.e.
To build on @stickies-v comment, terminals may have a history limit and so if the prefix was only for dropped lines, an attack (which should now be thwarted!) could go undetected.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190506106)
> But shouldn't we only prefix the dropped lines by * anyway (why prefix the lines that aren't dropped in the first place?), so why do we care about a global suppression state, i.e.
To build on @stickies-v comment, terminals may have a history limit and so if the prefix was only for dropped lines, an attack (which should now be thwarted!) could go undetected.
💬 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_r2190615792)
I think this is clear because there is no global rate limit in the code and only a source location rate limit.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190615792)
I think this is clear because there is no global rate limit in the code and only a source location rate limit.
💬 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_r2190465915)
This is mentioned in the PR description and also in the commit message of b5d0cc29ac378f360424472f637f6d6af74660a5.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190465915)
This is mentioned in the PR description and also in the commit message of b5d0cc29ac378f360424472f637f6d6af74660a5.
📝 ismaelsadeeq opened a pull request: "doc: fix `BlockConnected` incorrect comment"
(https://github.com/bitcoin/bitcoin/pull/32893)
This is a simple PR that fixes the `BlockConnected` validation interface notification comment, which incorrectly states that a vector of transactions removed from the mempool is as a parameter of the method.
Originally, this was the case when the method was first introduced in https://github.com/bitcoin/bitcoin/pull/9725
However, the method has since changed, and this is no longer accurate. Keeping the outdated comment is now misleading.
This PR removes the information about the method
...
(https://github.com/bitcoin/bitcoin/pull/32893)
This is a simple PR that fixes the `BlockConnected` validation interface notification comment, which incorrectly states that a vector of transactions removed from the mempool is as a parameter of the method.
Originally, this was the case when the method was first introduced in https://github.com/bitcoin/bitcoin/pull/9725
However, the method has since changed, and this is no longer accurate. Keeping the outdated comment is now misleading.
This PR removes the information about the method
...
📝 murchandamus opened a pull request: "FUZZ: Test that BnB finds best solution"
(https://github.com/bitcoin/bitcoin/pull/32894)
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
(https://github.com/bitcoin/bitcoin/pull/32894)
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3046089923)
Review Request: @achow101 @Sjors @maflcko
I have passed all cis, but bot did not remove "ci failed" label...
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3046089923)
Review Request: @achow101 @Sjors @maflcko
I have passed all cis, but bot did not remove "ci failed" label...
🤔 furszy reviewed a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2994877144)
I think something like this, if properly implemented (I haven't thought much about the code yet), would reduce the GUI freezes during IBD in a noticeable manner.
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2994877144)
I think something like this, if properly implemented (I haven't thought much about the code yet), would reduce the GUI freezes during IBD in a noticeable manner.
🤔 janb84 reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2994892882)
re ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Changes sinds last ACK:
- changes related to various suggestions
- help output improvements:
<details>
```shell
$ ./build/bin/bitcoin
Usage: bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show ful
...
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2994892882)
re ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Changes sinds last ACK:
- changes related to various suggestions
- help output improvements:
<details>
```shell
$ ./build/bin/bitcoin
Usage: bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show ful
...
💬 ismaelsadeeq commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3046135912)
> Yes, this should be also for submitpackage. The reason I did not implement it is because I want to cut this PR slim to get the private broadcast mechanism in, reduce review load, because it is already big as it is. There is a "Future extenstions" section the PR description, added submitpackage there and linked to your comment with the diff.
1. In that case, this probably shouldn’t be a startup option but rather a runtime option for `sendrawtransaction`, I think.
when we have privatebroadca
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3046135912)
> Yes, this should be also for submitpackage. The reason I did not implement it is because I want to cut this PR slim to get the private broadcast mechanism in, reduce review load, because it is already big as it is. There is a "Future extenstions" section the PR description, added submitpackage there and linked to your comment with the diff.
1. In that case, this probably shouldn’t be a startup option but rather a runtime option for `sendrawtransaction`, I think.
when we have privatebroadca
...
💬 ismaelsadeeq commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2190771746)
> "So using `sendrawtransaction` for manual rebroadcast will degrade privacy by leaking the transaction's origin."
> "This conceals the transaction origin."
What I meant is that this seems to go beyond the scope of API docs imo? Perhaps we should have a dedicated privacy section in the docs maybe something like `doc/privacy` which could include:
* The existing content from [[doc/tor.md#4-privacy-recommendations](https://github.com/bitcoin/bitcoin/blob/master/doc/tor.md#4-privacy-recomme
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2190771746)
> "So using `sendrawtransaction` for manual rebroadcast will degrade privacy by leaking the transaction's origin."
> "This conceals the transaction origin."
What I meant is that this seems to go beyond the scope of API docs imo? Perhaps we should have a dedicated privacy section in the docs maybe something like `doc/privacy` which could include:
* The existing content from [[doc/tor.md#4-privacy-recommendations](https://github.com/bitcoin/bitcoin/blob/master/doc/tor.md#4-privacy-recomme
...
🤔 janb84 reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2994960493)
re ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
changes since last ACK:
- rebase
- spelling/grammer fix
- changes due to suggestions (thanks !)
build & tested ✅
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2994960493)
re ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
changes since last ACK:
- rebase
- spelling/grammer fix
- changes due to suggestions (thanks !)
build & tested ✅
💬 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_r2190812498)
The PR description still states:
> Both the quota and time window are configurable
As far as I can tell that's not the case anymore.
> When logging is restarted a tally of how many messages/bytes were dropped is printed
Do we also print the message count?
----
> But if we stay with the per-line logging-quota, I think this should at least be explained in the PR description
I meant the reason for choosing to restrict based on source lines and not globally. I'm still not fully co
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190812498)
The PR description still states:
> Both the quota and time window are configurable
As far as I can tell that's not the case anymore.
> When logging is restarted a tally of how many messages/bytes were dropped is printed
Do we also print the message count?
----
> But if we stay with the per-line logging-quota, I think this should at least be explained in the PR description
I meant the reason for choosing to restrict based on source lines and not globally. I'm still not fully co
...