💬 achow101 commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-3309717927)
ACK b81f37031c8f2ccad9346f1b65ee0f8083c44796
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-3309717927)
ACK b81f37031c8f2ccad9346f1b65ee0f8083c44796
👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3241370035)
Code review ACK d1be44d3a672fc343eb3e64f4c0fea1a7e3030aa. Thanks for the update. I do feel like this is simpler with just one table. I left some suggestions below which are mostly about documentation and not important.
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3241370035)
Code review ACK d1be44d3a672fc343eb3e64f4c0fea1a7e3030aa. Thanks for the update. I do feel like this is simpler with just one table. I left some suggestions below which are mostly about documentation and not important.
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360621351)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Could consider dropping the RPCConvertTable class and g_rpc_convert_table global variable. The RPCConvertTable class has no state and only has two standalone methods which could be plain functions.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360621351)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Could consider dropping the RPCConvertTable class and g_rpc_convert_table global variable. The RPCConvertTable class has no state and only has two standalone methods which could be plain functions.
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361185088)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Just IMO, but I feel like the example is confusing because "key=store" looks like a case where "key" actually is a named parameter and = sign should be parsed, not a case where it shouldn't be parsed. Also the next two sentences seem to just be repeating what the first two sentences say without out adding anything new. And I'm not sure the "IMPORTANT" section is really so imp
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361185088)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Just IMO, but I feel like the example is confusing because "key=store" looks like a case where "key" actually is a named parameter and = sign should be parsed, not a case where it shouldn't be parsed. Also the next two sentences seem to just be repeating what the first two sentences say without out adding anything new. And I'm not sure the "IMPORTANT" section is really so imp
...
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361189357)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Maybe consider deleting this comment. It seems like it is mostly just repeating what code is doing without explaining why it is doing these things. If you do think a comment would be helpful here, probably the most useful thing might be some examples of the things being parsed. I suggested some documentation with examples in da919fb63c519ac91c9704c58f1bbe522d7a8879 ([tag](htt
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361189357)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Maybe consider deleting this comment. It seems like it is mostly just repeating what code is doing without explaining why it is doing these things. If you do think a comment would be helpful here, probably the most useful thing might be some examples of the things being parsed. I suggested some documentation with examples in da919fb63c519ac91c9704c58f1bbe522d7a8879 ([tag](htt
...
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361200492)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Note: with #33230 this could have JSON_OR_STRING added as well
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361200492)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Note: with #33230 this could have JSON_OR_STRING added as well
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360610696)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Could consider just leaving this function above the RPCConvertTable class instead of moving it below, since it is not changing. This would make the diff a little smaller.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360610696)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)
Could consider just leaving this function above the RPCConvertTable class instead of moving it below, since it is not changing. This would make the diff a little smaller.
💬 achow101 commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3309768622)
ACK 1a4ad0ae501d15f77b1601ace3e1a1c8bf440dd6
Tested on Windows 11, and it appears to correctly remove the old `(64-bit)` uninstall entry.
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3309768622)
ACK 1a4ad0ae501d15f77b1601ace3e1a1c8bf440dd6
Tested on Windows 11, and it appears to correctly remove the old `(64-bit)` uninstall entry.
⚠️ polespinasa opened an issue: "Use compact blocks while doing background validation"
(https://github.com/bitcoin/bitcoin/issues/33431)
We don’t use Compact Blocks while performing background validation if the `assumetxoutset` option was used.
After loading a snapshot and syncing all the way to the chain tip, Compact Blocks can be used for newly announced blocks because the mempool is already being populated even if doing background validation.
(https://github.com/bitcoin/bitcoin/issues/33431)
We don’t use Compact Blocks while performing background validation if the `assumetxoutset` option was used.
After loading a snapshot and syncing all the way to the chain tip, Compact Blocks can be used for newly announced blocks because the mempool is already being populated even if doing background validation.
💬 luke-jr commented on pull request "wallet/rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2361315826)
This shouldn't be a positional parameter, at least.
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2361315826)
This shouldn't be a positional parameter, at least.
🤔 luke-jr reviewed a pull request: "wallet/rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy"
(https://github.com/bitcoin/bitcoin/pull/33392#pullrequestreview-3242264016)
I agree getbalance* is the wrong place for this kind of check.
But it also will fail to detect incorrect birthdates if the TXOs are spent, so it can't be relied on either...
(https://github.com/bitcoin/bitcoin/pull/33392#pullrequestreview-3242264016)
I agree getbalance* is the wrong place for this kind of check.
But it also will fail to detect incorrect birthdates if the TXOs are spent, so it can't be relied on either...
🤔 l0rinc reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3242130677)
Thanks for the comments, adjusted the test and code to differentiate between cases when the block is after the assumevalid block from when it's not even on the same chain.
Also adjusted the style of the test to have more self-contained blocks separated by the `with` blocks, unified the test string styles, updated old commit messages.
Pushed the [change](https://github.com/bitcoin/bitcoin/compare/f66036d47e221bed7fe94c517631be920da66d96..fb5405e43acf6d0e6ce74643f6372410ef8cc41d) and the rebase se
...
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3242130677)
Thanks for the comments, adjusted the test and code to differentiate between cases when the block is after the assumevalid block from when it's not even on the same chain.
Also adjusted the style of the test to have more self-contained blocks separated by the `with` blocks, unified the test string styles, updated old commit messages.
Pushed the [change](https://github.com/bitcoin/bitcoin/compare/f66036d47e221bed7fe94c517631be920da66d96..fb5405e43acf6d0e6ce74643f6372410ef8cc41d) and the rebase se
...
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361341176)
I missed this comment somehow.
While this is a tiny change to the validation logic, I think we can make it more focused by putting it inside the original `GetAncestor(pindex->nHeight) != pindex` block.
Now that it was removed from V30 we can safely do this I think - and the messages would be a lot more useful this way. Added a new test to cover this and unified the test styles as well since it's not *that* urgent anymore.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361341176)
I missed this comment somehow.
While this is a tiny change to the validation logic, I think we can make it more focused by putting it inside the original `GetAncestor(pindex->nHeight) != pindex` block.
Now that it was removed from V30 we can safely do this I think - and the messages would be a lot more useful this way. Added a new test to cover this and unified the test styles as well since it's not *that* urgent anymore.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361340578)
Done
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361340578)
Done
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361210534)
Done
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2361210534)
Done
🤔 l0rinc requested changes to a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3241292554)
Thanks for reducing useless work - did you measure any speedup?
I think we could make the result simpler and more pythonic - it's a test, I find visibly simpler code better here than theoretically faster one that we can't even measure, so I suggested a few changes.
Here's the end-result containing most of my suggestions:
```patch
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index e28140c17b..ccbde4702c 100755
--- a/test/functional/t
...
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3241292554)
Thanks for reducing useless work - did you measure any speedup?
I think we could make the result simpler and more pythonic - it's a test, I find visibly simpler code better here than theoretically faster one that we can't even measure, so I suggested a few changes.
Here's the end-result containing most of my suggestions:
```patch
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index e28140c17b..ccbde4702c 100755
--- a/test/functional/t
...
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2360549564)
this was weird before, it logged `print_log` which was created in different scope. Having `log` here is still weird, but I guess Python is weird.
nit: based on the docs, [`!r`](https://docs.python.org/3/library/functions.html#repr) might be more appropriate here:
```suggestion
self._raise_assertion_error(f'Expected message(s) {expected_msgs!r} not found in log:\n\n{print_log(log)}\n\n')
```
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2360549564)
this was weird before, it logged `print_log` which was created in different scope. Having `log` here is still weird, but I guess Python is weird.
nit: based on the docs, [`!r`](https://docs.python.org/3/library/functions.html#repr) might be more appropriate here:
```suggestion
self._raise_assertion_error(f'Expected message(s) {expected_msgs!r} not found in log:\n\n{print_log(log)}\n\n')
```
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2360551867)
nit: you could mention the reason for changing the texts as well
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2360551867)
nit: you could mention the reason for changing the texts as well
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2361380779)
what's the reason for regex iteration above and here if we're just escaping it anyway? We're not doing it in `busy_wait_for_debug_log`
I have added
```python
found = []
for i, expected_msg in enumerate(expected_msgs):
if re.search(re.escape(expected_msg), log, flags=re.MULTILINE):
found.append(i)
found2 = [i for i, m in enumerate(expected_msgs) if m in log]
assert found == found2, f"Mismatch: {found} != {
...
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2361380779)
what's the reason for regex iteration above and here if we're just escaping it anyway? We're not doing it in `busy_wait_for_debug_log`
I have added
```python
found = []
for i, expected_msg in enumerate(expected_msgs):
if re.search(re.escape(expected_msg), log, flags=re.MULTILINE):
found.append(i)
found2 = [i for i, m in enumerate(expected_msgs) if m in log]
assert found == found2, f"Mismatch: {found} != {
...
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2361431692)
Wouldn't this return if the same expected message appears multiple times in the log? I think we should collect the indexes into a set instead.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2361431692)
Wouldn't this return if the same expected message appears multiple times in the log? I think we should collect the indexes into a set instead.