💬 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
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614076142)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605689279
> llm nit in [6c98ddc](https://github.com/bitcoin/bitcoin/commit/6c98ddc87a895d9bbc392db8fedc0e8268ff1cdd): could use named args while touching?
Makes sense, added.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614076142)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605689279
> llm nit in [6c98ddc](https://github.com/bitcoin/bitcoin/commit/6c98ddc87a895d9bbc392db8fedc0e8268ff1cdd): could use named args while touching?
Makes sense, added.
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614096306)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605695970
> i'd say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes
Yeah I think it needs to be up to callers to check their own expected states or to check the return status of this function. Having this function make unnecessary assumptions about particular states it thinks it will be called in seems like it could be easil
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614096306)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605695970
> i'd say assumes are fine to detect logic bugs or storage corruption, but no strong opinion, if the goal is to somehow make this more flexible for future changes
Yeah I think it needs to be up to callers to check their own expected states or to check the return status of this function. Having this function make unnecessary assumptions about particular states it thinks it will be called in seems like it could be easil
...