💬 tdb3 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1631816452)
Unless I'm missing something, this is functionally identical to `TestNode.replace_in_config()`. Using `replace_in_config()` instead could make things shorter by reusing code. For example:
```diff
diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
index 55cba41754..f65b13a440 100755
--- a/test/functional/rpc_users.py
+++ b/test/functional/rpc_users.py
@@ -110,12 +110,7 @@ class HTTPBasicsTest(BitcoinTestFramework):
assert_equal(expected_perms, actual_
...
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1631816452)
Unless I'm missing something, this is functionally identical to `TestNode.replace_in_config()`. Using `replace_in_config()` instead could make things shorter by reusing code. For example:
```diff
diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
index 55cba41754..f65b13a440 100755
--- a/test/functional/rpc_users.py
+++ b/test/functional/rpc_users.py
@@ -110,12 +110,7 @@ class HTTPBasicsTest(BitcoinTestFramework):
assert_equal(expected_perms, actual_
...
🚀 fanquake merged a pull request: "guix: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126"
(https://github.com/bitcoin/bitcoin/pull/30231)
(https://github.com/bitcoin/bitcoin/pull/30231)
🚀 fanquake merged a pull request: "json-rpc 2.0 followups: docs, tests, cli"
(https://github.com/bitcoin/bitcoin/pull/30238)
(https://github.com/bitcoin/bitcoin/pull/30238)
💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2155888680)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2155888680)
Concept ACK
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2155889026)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2155889026)
Concept ACK
🤔 hebasto reviewed a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244#pullrequestreview-2105826541)
Tested 3d976000afccac4e89931496f23cfe8593daeb75 on Ubuntu 24.04.
Passing `--exclude "rpc_bind.py --ipv6"` excludes not only the specified test but also `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback`.
(https://github.com/bitcoin/bitcoin/pull/30244#pullrequestreview-2105826541)
Tested 3d976000afccac4e89931496f23cfe8593daeb75 on Ubuntu 24.04.
Passing `--exclude "rpc_bind.py --ipv6"` excludes not only the specified test but also `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback`.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632035840)
I've added a few preparatory commits before this one, please have a look.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632035840)
I've added a few preparatory commits before this one, please have a look.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632036023)
Done, in an additional commit.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632036023)
Done, in an additional commit.
🤔 fjahr reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2105839640)
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
I also confirmed that the new test fails before the changes here are applied.
I think the test could use a bit more explanation on its reasoning but this shouldn't block a merge of this as-is.
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2105839640)
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
I also confirmed that the new test fails before the changes here are applied.
I think the test could use a bit more explanation on its reasoning but this shouldn't block a merge of this as-is.
💬 fjahr commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632034546)
nit: Why was the blockfilterindex chosen here? I am assuming because it's slow? Would be good to add a comment because it may be confusing for others in the future what this choice has to do with the test.
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632034546)
nit: Why was the blockfilterindex chosen here? I am assuming because it's slow? Would be good to add a comment because it may be confusing for others in the future what this choice has to do with the test.
💬 fjahr commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632036240)
nit: I guess this is needed so the node can be stopped fast enough. It's still a race and could turn out to be flakey in the CI, right? I don't have a better idea to fix this right now but a comment might be good to make this explicit and make future debugging easier if this turns out to be the case.
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632036240)
nit: I guess this is needed so the node can be stopped fast enough. It's still a race and could turn out to be flakey in the CI, right? I don't have a better idea to fix this right now but a comment might be good to make this explicit and make future debugging easier if this turns out to be the case.
💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632042991)
I just picked the one which furszy picked and I'm guessing he just picked one too. I'll check if there is any signifcant effect when picking a different one.
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632042991)
I just picked the one which furszy picked and I'm guessing he just picked one too. I'll check if there is any signifcant effect when picking a different one.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632043452)
Seems I did not. Done now.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632043452)
Seems I did not. Done now.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632043992)
It was added in #26153, with a commit originally taken from #25325. I assume it originally had an explicit `copy()` member function, which was removed before merge. I've added a commit to remove it.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632043992)
It was added in #26153, with a commit originally taken from #25325. I assume it originally had an explicit `copy()` member function, which was removed before merge. I've added a commit to remove it.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2156037857)
I've rebased on top of the now-merged #30161, and believe I've addressed outstanding comments.
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2156037857)
I've rebased on top of the now-merged #30161, and believe I've addressed outstanding comments.
📝 m3dwards converted_to_draft a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244)
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
(https://github.com/bitcoin/bitcoin/pull/30244)
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
💬 m3dwards commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2156093176)
Converting back to draft to see if I can fix the bug @hebasto found.
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2156093176)
Converting back to draft to see if I can fix the bug @hebasto found.
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622933175)
typo: instead
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622933175)
typo: instead
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1623132784)
Why call `PruneBlockIndexCandidates` twice instead of moving it here?
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1623132784)
Why call `PruneBlockIndexCandidates` twice instead of moving it here?
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622932918)
typo: replace
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1622932918)
typo: replace
💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632134959)
If I am understanding this logic correctly, if `m_deterministic_prng.has_value()` is true then MixExtract in SeedStrengthen will never use m_counter or m_state to do MixExtract?
Strengthen is going to make the output of this call non-deterministic anyways, so what value does `allow_deterministic=true` as an argument provide in MixExtract? Shouldn't this be false?
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632134959)
If I am understanding this logic correctly, if `m_deterministic_prng.has_value()` is true then MixExtract in SeedStrengthen will never use m_counter or m_state to do MixExtract?
Strengthen is going to make the output of this call non-deterministic anyways, so what value does `allow_deterministic=true` as an argument provide in MixExtract? Shouldn't this be false?