💬 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
...
💬 davidgumberg commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3046200634)
> Not sure I understand this - why would the block be part of the active chain by that time? We started a request in the first place because we don't know the full block yet, and if that request has timed out, chances are we still don't have the block, in which case it won't be part of our active chain. Or do you count on a parallel compact-block download being successful in the meantime?
My assumption was that it was pretty unlikely for it to ever take >= 10 minutes to receive a block after
...
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3046200634)
> Not sure I understand this - why would the block be part of the active chain by that time? We started a request in the first place because we don't know the full block yet, and if that request has timed out, chances are we still don't have the block, in which case it won't be part of our active chain. Or do you count on a parallel compact-block download being successful in the meantime?
My assumption was that it was pretty unlikely for it to ever take >= 10 minutes to receive a block after
...
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3046202153)
>>Sending own transactions over long lived connections is problematic because then two different transactions that are otherwise unrelated, looking at the coins (ie on-chain analysis) could be linked together to the same originator.
>Is it not possible to perform this type of on-chain analysis even with privatebroadcast? For example, someone could observe the blockchain and fingerprint transactions as likely coming from a particular node and label it as "vasilsnode"?
Consider a wallet with
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3046202153)
>>Sending own transactions over long lived connections is problematic because then two different transactions that are otherwise unrelated, looking at the coins (ie on-chain analysis) could be linked together to the same originator.
>Is it not possible to perform this type of on-chain analysis even with privatebroadcast? For example, someone could observe the blockchain and fingerprint transactions as likely coming from a particular node and label it as "vasilsnode"?
Consider a wallet with
...
💬 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_r2190813991)
Ok, if you're fine with that, I'm fine with that, let's do the remaining ones in follow-ups.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190813991)
Ok, if you're fine with that, I'm fine with that, let's do the remaining ones in follow-ups.
💬 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_r2190816506)
Fair points, maybe we could hint at that in the first message which announces that some logs are being suppressed (explaining what the new `*` prefix indicates).
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190816506)
Fair points, maybe we could hint at that in the first message which announces that some logs are being suppressed (explaining what the new `*` prefix indicates).
💬 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_r2190822287)
> Recursion is not inherently bad
When the stopping condition isn't rock solit, it kinda' is. Maybe it is in this case - but given that we had to add a commit to reassure the readers, it may not be. I'd sleep better knowing that we haven't just introduced a new attack vector...
> What do you suggest?
In other cases we've switched recursive methods to iterative ones, e.g. https://github.com/bitcoin/bitcoin/pull/32351. I haven't investigated what that would imply here, got already tired a
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2190822287)
> Recursion is not inherently bad
When the stopping condition isn't rock solit, it kinda' is. Maybe it is in this case - but given that we had to add a commit to reassure the readers, it may not be. I'd sleep better knowing that we haven't just introduced a new attack vector...
> What do you suggest?
In other cases we've switched recursive methods to iterative ones, e.g. https://github.com/bitcoin/bitcoin/pull/32351. I haven't investigated what that would imply here, got already tired a
...
👍 l0rinc approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2995010190)
While there are still more nits left than I'm comfortable with, I understand wanting to have some progress - and the current PR is indeed an improvement.
Lightly tested code review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
I'm looking forward to ironing out the wrinkles in follow-up PRs.
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2995010190)
While there are still more nits left than I'm comfortable with, I understand wanting to have some progress - and the current PR is indeed an improvement.
Lightly tested code review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
I'm looking forward to ironing out the wrinkles in follow-up PRs.
💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3046267831)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3046267831)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f