💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2190631992)
Yeah, the call should be outside the if statement because the fee estimator decays previously tracked data points after block connection in order to make stale data points less relevant and recent data points more relevant over time.
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2190631992)
Yeah, the call should be outside the if statement because the fee estimator decays previously tracked data points after block connection in order to make stale data points less relevant and recent data points more relevant over time.
💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2190634605)
On what I discussed with @furszy I posted comment here https://github.com/bitcoin/bitcoin/pull/32748#discussion_r2187355851
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2190634605)
On what I discussed with @furszy I posted comment here https://github.com/bitcoin/bitcoin/pull/32748#discussion_r2187355851
💬 Sjors commented on pull request "rpc: use anti-fee-sniping by default":
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3045926086)
Expanded the existing `test_anti_fee_sniping` case in `wallet_create_tx.py` to cover `send` `sendall`. Making `sendall` actually work required an additional refactor.
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3045926086)
Expanded the existing `test_anti_fee_sniping` case in `wallet_create_tx.py` to cover `send` `sendall`. Making `sendall` actually work required an additional refactor.
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-3045929125)
Rebased and addressed comment from @DrahtBot see [diff](https://github.com/bitcoin/bitcoin/compare/b487edcffab9ace19fd927998d82e945bd39b0a4..3cde7f185255b1bc07a90e95708665e0b6d5a46d)
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-3045929125)
Rebased and addressed comment from @DrahtBot see [diff](https://github.com/bitcoin/bitcoin/compare/b487edcffab9ace19fd927998d82e945bd39b0a4..3cde7f185255b1bc07a90e95708665e0b6d5a46d)
💬 ismaelsadeeq commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#issuecomment-3045932178)
Rebased to address conflict [diff](https://github.com/bitcoin/bitcoin/compare/bad72f1a24585768f4bac937a17597879063400e..f9b811cb041c14d514f3e41bc8c944b83b37e066)
(https://github.com/bitcoin/bitcoin/pull/31664#issuecomment-3045932178)
Rebased to address conflict [diff](https://github.com/bitcoin/bitcoin/compare/bad72f1a24585768f4bac937a17597879063400e..f9b811cb041c14d514f3e41bc8c944b83b37e066)
💬 mzumsande commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3045975801)
> disconnect peers after the time-out period if the BlockRequested is part of the active chain
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?
> But, ma
...
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3045975801)
> disconnect peers after the time-out period if the BlockRequested is part of the active chain
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?
> But, ma
...
🤔 stickies-v reviewed a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2994752288)
Conceptually not a bad idea to cache and lock less, but imo this makes the code more brittle (and harder to understand), e.g. if any tip updates happen without the cache being updated separately.
Do you have any data as to the actual performance improvements from this PR?
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2994752288)
Conceptually not a bad idea to cache and lock less, but imo this makes the code more brittle (and harder to understand), e.g. if any tip updates happen without the cache being updated separately.
Do you have any data as to the actual performance improvements from this PR?
💬 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.