✅ fanquake closed a pull request: "p2p: avoid traversing blocks (twice) during IBD"
(https://github.com/bitcoin/bitcoin/pull/32730)
(https://github.com/bitcoin/bitcoin/pull/32730)
🚀 fanquake merged a pull request: "net: Waste less time in socket handling"
(https://github.com/bitcoin/bitcoin/pull/34025)
(https://github.com/bitcoin/bitcoin/pull/34025)
💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3645987726)
re-ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
No changes except improved self-documentation of test, ty!
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3645987726)
re-ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
No changes except improved self-documentation of test, ty!
👍 rkrux approved a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3571354142)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
This is helpful. Having gone through the logging code recently, I found myself confused a bit upon seeing different ways to log. Removal of `LogPrintLevel` helps in decreasing the confusion by just using `LOG<$LEVEL>()` macros.
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3571354142)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63
This is helpful. Having gone through the logging code recently, I found myself confused a bit upon seeing different ways to log. Removal of `LogPrintLevel` helps in decreasing the confusion by just using `LOG<$LEVEL>()` macros.
💬 stickies-v commented on pull request "lint: Remove confusing, redundant, and brittle lint-spelling":
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645998330)
Concept ACK. Didn't seem to be a useful test, so best to remove it.
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645998330)
Concept ACK. Didn't seem to be a useful test, so best to remove it.
📝 optout21 converted_to_draft a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).
The motivation is to document the skip value computation through a test.
The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.
The second commit adds low-level tests to the `GetSkipH
...
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).
The motivation is to document the skip value computation through a test.
The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.
The second commit adds low-level tests to the `GetSkipH
...
⚠️ husstege-e opened an issue: "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)"
(https://github.com/bitcoin/bitcoin/issues/34056)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
When using Replace-By-Fee (RBF) in combination with wallet rescans, the wallet RPC reports transactions with negative confirmation counts (e.g. -900, -1200), long after the replacing transaction has been confirmed.
These negative confirmation values persist even when:
- the replacement transaction is deeply confirmed
- the original transaction is fully conflicted and no longer in the memp
...
(https://github.com/bitcoin/bitcoin/issues/34056)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
When using Replace-By-Fee (RBF) in combination with wallet rescans, the wallet RPC reports transactions with negative confirmation counts (e.g. -900, -1200), long after the replacing transaction has been confirmed.
These negative confirmation values persist even when:
- the replacement transaction is deeply confirmed
- the original transaction is fully conflicted and no longer in the memp
...
👍 rkrux approved a pull request: "lint: Remove confusing, redundant, and brittle lint-spelling"
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3571429071)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3571429071)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613872871)
I still don't see the point of leaving the `Assume` in there since it seems to make no difference other than constricting somebody that would want to create a new macro for `LogInfo` w category. But i wont hold the PR back because of it.
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613872871)
I still don't see the point of leaving the `Assume` in there since it seems to make no difference other than constricting somebody that would want to create a new macro for `LogInfo` w category. But i wont hold the PR back because of it.
💬 rkrux commented on pull request "wallet, doc: clarify the coin selection filters that enforce cluster count":
(https://github.com/bitcoin/bitcoin/pull/34037#issuecomment-3646124212)
> Since https://github.com/bitcoin/bitcoin/pull/33639, these filters have started using cluster count instead of max desc count across ancestors.
The linked PR doesn't seem correct to me. Is it supposed to be #33629 instead?
(https://github.com/bitcoin/bitcoin/pull/34037#issuecomment-3646124212)
> Since https://github.com/bitcoin/bitcoin/pull/33639, these filters have started using cluster count instead of max desc count across ancestors.
The linked PR doesn't seem correct to me. Is it supposed to be #33629 instead?
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3646162211)
Rebased and fixed conflict.
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3646162211)
Rebased and fixed conflict.
💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613946723)
> I still don't see the point of leaving the `Assume` in there since it seems to make no difference
The point of the `Assume` is to document internal assumptions for developers. The docs say `No rate limiting is applied, `. So writing an `Assume(!rate_limit)` ensures this to some extent for developers and documents it further.
The assume is just a trivial single line, I don't see the risk to keep it. It is also more trivial to remove, than to update the doc string itself.
I'll leave as-
...
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613946723)
> I still don't see the point of leaving the `Assume` in there since it seems to make no difference
The point of the `Assume` is to document internal assumptions for developers. The docs say `No rate limiting is applied, `. So writing an `Assume(!rate_limit)` ensures this to some extent for developers and documents it further.
The assume is just a trivial single line, I don't see the risk to keep it. It is also more trivial to remove, than to update the doc string itself.
I'll leave as-
...
👋 optout21's pull request is ready for review: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
(https://github.com/bitcoin/bitcoin/pull/33661)
✅ maflcko closed an issue: "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)"
(https://github.com/bitcoin/bitcoin/issues/34056)
(https://github.com/bitcoin/bitcoin/issues/34056)
💬 maflcko commented on issue "Wallet RPC semantics: gettransaction.confirmations returns large negative values for RBF-replaced transactions (undocumented behavior)":
(https://github.com/bitcoin/bitcoin/issues/34056#issuecomment-3646182093)
> undocumented
To get the documentation, you can use the `help` RPC. It will explain and document the output. E.g.
```
"confirmations" : n, (numeric) The number of confirmations for the transaction. Negative confirmations means the
transaction conflicted that many blocks ago.
(https://github.com/bitcoin/bitcoin/issues/34056#issuecomment-3646182093)
> undocumented
To get the documentation, you can use the `help` RPC. It will explain and document the output. E.g.
```
"confirmations" : n, (numeric) The number of confirmations for the transaction. Negative confirmations means the
transaction conflicted that many blocks ago.
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646190934)
I redid the test, following @sipa's remark.
- The test on skip height properties -- median, avg -- is dropped
- A new test is added, that tests the skip heights _indirectly_, by doing a bunch of ancestor searches, and counting the number of steps needed, and doing some asserts on the step count. In a 200k long chain a series 1000 runs are performed. In each run a start and a target heights are picked randomly: the start is in the upper half of the chain, and the target is below it, by at lea
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646190934)
I redid the test, following @sipa's remark.
- The test on skip height properties -- median, avg -- is dropped
- A new test is added, that tests the skip heights _indirectly_, by doing a bunch of ancestor searches, and counting the number of steps needed, and doing some asserts on the step count. In a 200k long chain a series 1000 runs are performed. In each run a start and a target heights are picked randomly: the start is in the upper half of the chain, and the target is below it, by at lea
...
💬 maflcko commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-3646334143)
Only change is rebase and fix of two LLM nits.
re-review ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496 🕍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNL
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-3646334143)
Only change is rebase and fix of two LLM nits.
re-review ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496 🕍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNL
...
👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3571618055)
re-ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3571618055)
re-ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614058989)
nit: Agree on reducing the number of direct conditions by using `SaturatingAdd` as sugggested at the beginning of the thread (passes current unit tests).
No strong opinion for/against disallowing 0-size ranges.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614058989)
nit: Agree on reducing the number of direct conditions by using `SaturatingAdd` as sugggested at the beginning of the thread (passes current unit tests).
No strong opinion for/against disallowing 0-size ranges.
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614038938)
(meganit: Would add the comment in the first commit instead of the last if you re-touch).
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614038938)
(meganit: Would add the comment in the first commit instead of the last if you re-touch).
🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3571658129)
Thanks for the reviews!
<!-- begin push-24 -->
Rebased 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c -> 82be652e40ec7e1bea4b260ee804a92a3e05f496 ([`pr/cstate.23`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.23) -> [`pr/cstate.24`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.23-rebase..pr/cstate.24))<!-- end --> fixing conflict #32414 and also adding suggested arg name comments
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3571658129)
Thanks for the reviews!
<!-- begin push-24 -->
Rebased 5dd7cbf675e7ce058ccb0666dabcb39a175ae83c -> 82be652e40ec7e1bea4b260ee804a92a3e05f496 ([`pr/cstate.23`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.23) -> [`pr/cstate.24`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.23-rebase..pr/cstate.24))<!-- end --> fixing conflict #32414 and also adding suggested arg name comments