💬 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
...
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614074730)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2603120305
> q in [cb3e7af](https://github.com/bitcoin/bitcoin/commit/cb3e7af4ed04295e9928c2b60d8ab4ba64c4efd5): I wonder if this should be fixed in a follow-up, because the comment in `AttachChain` says that the loop called when `hasAssumedValidChain` is true may be slow.
Yes it could be nice to change this and make loading old wallets on non-pruned nodes faster, even though it's a little bit of an edge case
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614074730)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2603120305
> q in [cb3e7af](https://github.com/bitcoin/bitcoin/commit/cb3e7af4ed04295e9928c2b60d8ab4ba64c4efd5): I wonder if this should be fixed in a follow-up, because the comment in `AttachChain` says that the loop called when `hasAssumedValidChain` is true may be slow.
Yes it could be nice to change this and make loading old wallets on non-pruned nodes faster, even though it's a little bit of an edge case
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614077388)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605693370
> llm nit in the same commit: name args?
Thanks also added this
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2614077388)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2605693370
> llm nit in the same commit: name args?
Thanks also added this
💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614119273)
> nit: Agree on reducing the number of direct conditions by using `SaturatingAdd` as sugggested at the beginning of the thread (passes current unit tests).
>
Let's leave the nits for a follow-up? The thread has more than 250 comments, and at least on my end, I am having difficulties loading, reading, and writing new comments, due to GitHub slowness or brittleness.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2614119273)
> nit: Agree on reducing the number of direct conditions by using `SaturatingAdd` as sugggested at the beginning of the thread (passes current unit tests).
>
Let's leave the nits for a follow-up? The thread has more than 250 comments, and at least on my end, I am having difficulties loading, reading, and writing new comments, due to GitHub slowness or brittleness.
🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-3063099001)
<!-- begin push-35 -->
Updated 2cb5e6472c8c0109390258c298915dd4011f0292 -> 057ea0a677293b6ecaa48008228ca3086ab9e0ff ([`pr/mine.34`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.34) -> [`pr/mine.35`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.35), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.34..pr/mine.35))<!-- end --> just tweaking bitcoin-mine status message
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-3063099001)
<!-- begin push-35 -->
Updated 2cb5e6472c8c0109390258c298915dd4011f0292 -> 057ea0a677293b6ecaa48008228ca3086ab9e0ff ([`pr/mine.34`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.34) -> [`pr/mine.35`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.35), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.34..pr/mine.35))<!-- end --> just tweaking bitcoin-mine status message
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236751665)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157
+1. I think just having simple code to demonstrate the API with comments pointing out how usage could be more robust is a good middle ground.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236751665)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157
+1. I think just having simple code to demonstrate the API with comments pointing out how usage could be more robust is a good middle ground.
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236767287)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229432505
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2236767287)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229432505
Thanks, fixed
💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2614116248)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502
> In [1a844b8](https://github.com/bitcoin/bitcoin/commit/1a844b8ffd351d14b59de4cbae99b6e639068bb9) _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
I guess technically the message didn't say who mined the block. But reworded it to avoid unnecessary excitement
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2614116248)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502
> In [1a844b8](https://github.com/bitcoin/bitcoin/commit/1a844b8ffd351d14b59de4cbae99b6e639068bb9) _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
I guess technically the message didn't say who mined the block. But reworded it to avoid unnecessary excitement
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646374389)
[Since my last ACK](https://github.com/bitcoin/bitcoin/compare/389eafc631733c1ac2890e1b012a94f66a31ccad..07135290c1720a14c9d2f18a5700bb6565ae7a10): deduplicated and sorted the unit test cases, moved non-const return value in rpc `GetRawBlockChecked`, added default switch comment, renamed functional test lambda, and changed functional test category to be delimited by log instead of comment.
ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646374389)
[Since my last ACK](https://github.com/bitcoin/bitcoin/compare/389eafc631733c1ac2890e1b012a94f66a31ccad..07135290c1720a14c9d2f18a5700bb6565ae7a10): deduplicated and sorted the unit test cases, moved non-const return value in rpc `GetRawBlockChecked`, added default switch comment, renamed functional test lambda, and changed functional test category to be delimited by log instead of comment.
ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10