Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1631724039)
I think any attempt to invoke this comparison operator would just have failed, as `BitSet` doesn't have an `operator<=>` (it did at some point, but I had dropped it). I've just removed the operator here too.
💬 luke-jr 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_r1631747986)
0 or 1
💬 luke-jr 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_r1631748235)
Actually, I guess the 0s should be unique so this just needs rephrasing some other way
💬 luke-jr commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2155749633)
>Since these are test-network-only changes, are we required to preserve current behavior?

Whether you are required to or not, it is the correct thing to do. This option originated in Knots where it has always been available on mainnet, and expected to not enable upgradable opcodes/etc.
🤔 tdb3 reviewed a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2105557076)
Approach ACK.
This is looking great.

Left a comment in `test_rpccookieperms()`.

Also manually checked cookie file permissions after starting bitcoind without `-rpccookieperms` and with each option (`owner`, `group`, `all`) respectively. Permissions were as expected:
```
-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
```
```
-rw------- 1 dev dev 75 Jun 7 21:59 .cookie
```
```
-rw-r----- 1 dev dev 75 Jun 7 22:00 .cookie
```
```
-rw-r--r-- 1 dev dev 75 Jun 7 22:00 .cook
...
💬 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_
...
🚀 fanquake merged a pull request: "guix: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126"
(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)
💬 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
💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(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`.
💬 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.
💬 sipa commented on pull request "Several randomness improvements":
(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.
💬 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.
💬 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.
💬 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.
💬 sipa commented on pull request "Several randomness improvements":
(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.
💬 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.