💬 instagibbs commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2491873388)
minimal test case for pruning, feel free to take: https://gist.github.com/instagibbs/cb1f7cfbd0a70e8b5393e457c7a959e6
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2491873388)
minimal test case for pruning, feel free to take: https://gist.github.com/instagibbs/cb1f7cfbd0a70e8b5393e457c7a959e6
💬 jonatack commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491945225)
> Enforce this in the CI, having it to detect non-local traffic and fail accordingly.
Concept ACK
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491945225)
> Enforce this in the CI, having it to detect non-local traffic and fail accordingly.
Concept ACK
🤔 BrandonOdiwuor reviewed a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2452391378)
tACK b3a96b35691f3fbf958958056cd905e119fb48fe
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2452391378)
tACK b3a96b35691f3fbf958958056cd905e119fb48fe
💬 instagibbs commented on pull request "test: Deduplicate assert_mempool_contents()":
(https://github.com/bitcoin/bitcoin/pull/31338#issuecomment-2492087451)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
thanks for opening this
(https://github.com/bitcoin/bitcoin/pull/31338#issuecomment-2492087451)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638
thanks for opening this
🤔 jonatack reviewed a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2452455980)
Tested ACK b3a96b35691f3fbf958958056cd905e119fb48fe
With respect to #31161, the bug can be fixed now with this change and that pull easily updated if it moves forward.
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2452455980)
Tested ACK b3a96b35691f3fbf958958056cd905e119fb48fe
With respect to #31161, the bug can be fixed now with this change and that pull easily updated if it moves forward.
💬 hebasto commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1852773687)
> TODO:
>
> * [ ] fix `unknown target CPU 'armv8-a+crc+crypto'` [failure](https://cirrus-ci.com/task/5887765378760704) and re-enable macOS cross
Similar to https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2483394112, I do think that `-DWITH_MULTIPROCESS=OFF` can be dropped now as depends are [fixed](https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1848171448).
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1852773687)
> TODO:
>
> * [ ] fix `unknown target CPU 'armv8-a+crc+crypto'` [failure](https://cirrus-ci.com/task/5887765378760704) and re-enable macOS cross
Similar to https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2483394112, I do think that `-DWITH_MULTIPROCESS=OFF` can be dropped now as depends are [fixed](https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1848171448).
💬 mzumsande commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2492157450)
> Honestly, I forgot which test was that.
Maybe it was https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 (I independently ran into that a few weeks ago and included a fix in #31142)
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2492157450)
> Honestly, I forgot which test was that.
Maybe it was https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 (I independently ran into that a few weeks ago and included a fix in #31142)
📝 Parkeragan opened a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
(https://github.com/bitcoin/bitcoin/pull/31342)
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
✅ willcl-ark closed a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)
(https://github.com/bitcoin/bitcoin/pull/31342)
📝 fanquake locked a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
(https://github.com/bitcoin/bitcoin/pull/31342)
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2492366799)
Thank you for the reviews and the test case @instagibbs,
Updated 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 -> 73db95c65c1d372822166045ca8b9f173d5fd883 ([submitblock_prechecks_3](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_3) -> [submitblock_prechecks_4](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/submitblock_prechecks_3..submitblock_prechecks_4))
* Addressed @Sjors' [comment](https://
...
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2492366799)
Thank you for the reviews and the test case @instagibbs,
Updated 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 -> 73db95c65c1d372822166045ca8b9f173d5fd883 ([submitblock_prechecks_3](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_3) -> [submitblock_prechecks_4](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/submitblock_prechecks_3..submitblock_prechecks_4))
* Addressed @Sjors' [comment](https://
...
👍 TheCharlatan approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2452805746)
ACK 027ce3e9634b2f47c508899942d05690572de516
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2452805746)
ACK 027ce3e9634b2f47c508899942d05690572de516
💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852949174)
> This could also be because the user might not have built the binaries yet even in the case of default build path.
Yes, I haven't added it to the message because I though it would be too verbose, but I can reconsider it.
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852949174)
> This could also be because the user might not have built the binaries yet even in the case of default build path.
Yes, I haven't added it to the message because I though it would be too verbose, but I can reconsider it.
👍 hodlinator approved a pull request: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2452817327)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12
Mostly responded to my feedback since my last review.
Compiled with BDB and passed functional tests, unittests and ran `ephemeral_package_eval` fuzz target for 1 minute.
Still have [reservations about the prioritisetransaction RPC](https://github.com/bitcoin/bitcoin/pull/31279/files#r1848453460), but not blocking.
(https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2452817327)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12
Mostly responded to my feedback since my last review.
Compiled with BDB and passed functional tests, unittests and ran `ephemeral_package_eval` fuzz target for 1 minute.
Still have [reservations about the prioritisetransaction RPC](https://github.com/bitcoin/bitcoin/pull/31279/files#r1848453460), but not blocking.
🤔 hebasto reviewed a pull request: "guix: swap `moreutils` for just `sponge`"
(https://github.com/bitcoin/bitcoin/pull/31323#pullrequestreview-2452825209)
My Guix build:
```
riscv64
74a02707614d7fc7ed460148b37e90c126d09c2e9b2962765de14d267a3f22d3 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/SHA256SUMS.part
263f079da730485eb583995c1fc6304b7ca957a668fd5aac8f879bc59746dc67 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu-debug.tar.gz
745a9bfdd9550cf9abf80415c4efe818446ea05e9c4f659f643669c3dcb49893 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu.tar.gz
5810b1ef
...
(https://github.com/bitcoin/bitcoin/pull/31323#pullrequestreview-2452825209)
My Guix build:
```
riscv64
74a02707614d7fc7ed460148b37e90c126d09c2e9b2962765de14d267a3f22d3 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/SHA256SUMS.part
263f079da730485eb583995c1fc6304b7ca957a668fd5aac8f879bc59746dc67 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu-debug.tar.gz
745a9bfdd9550cf9abf80415c4efe818446ea05e9c4f659f643669c3dcb49893 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu.tar.gz
5810b1ef
...
💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852957327)
I've updated it, thank for the suggestion.
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852957327)
I've updated it, thank for the suggestion.
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852962780)
`-version=true` and `-version=false` are both interpreted as `-version=0` (there is no special parsing for booleans expressed as strings).
`-version=0` is equivalent to `-noversion` or `-noversion=1`.
`IsArgSet()` returns `true` if the user has set a given arg to *any value* (including negation), through command line or settings-file.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852962780)
`-version=true` and `-version=false` are both interpreted as `-version=0` (there is no special parsing for booleans expressed as strings).
`-version=0` is equivalent to `-noversion` or `-noversion=1`.
`IsArgSet()` returns `true` if the user has set a given arg to *any value* (including negation), through command line or settings-file.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2492442146)
Updated 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 -> 5910f9bb18da35101144304d385618d7b82420f5 ([kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2) -> [kernelApi_3](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_2..kernelApi_3))
* As discussed with @stickies-v out of band, make callbacks only return `const` pointers, which further ensures that the user does not de-allocate or take ownership of th
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2492442146)
Updated 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 -> 5910f9bb18da35101144304d385618d7b82420f5 ([kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2) -> [kernelApi_3](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_2..kernelApi_3))
* As discussed with @stickies-v out of band, make callbacks only return `const` pointers, which further ensures that the user does not de-allocate or take ownership of th
...
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852982119)
What would you use as category at the beginning of commit messages when implementations are merged with tests? - Feels logical to have for example `net: ...` commit + `test net: ...` commit, you would just have the first message and merge them together?
If the pre-existing tests were to be broken by changing the implementations, I can understand changing them in the same commit, but AFAIK that is not the case in this PR.
My impression is that this project encourages separate commits for te
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852982119)
What would you use as category at the beginning of commit messages when implementations are merged with tests? - Feels logical to have for example `net: ...` commit + `test net: ...` commit, you would just have the first message and merge them together?
If the pre-existing tests were to be broken by changing the implementations, I can understand changing them in the same commit, but AFAIK that is not the case in this PR.
My impression is that this project encourages separate commits for te
...
📝 theStack opened a pull request: "test: avoid internet traffic in rpc_net.py"
(https://github.com/bitcoin/bitcoin/pull/31343)
Rather than opening a connection to a public IP, use one in the [link-local address range](https://en.wikipedia.org/wiki/Link-local_address#IPv4) in order to avoid the problems described in #31339. This is still not ideal and could be annoying to users if there is an actual machine reachable on this address, so I'm open for better suggestions. As far as I understand, we don't want to use localhost once again here, since for testing the filtering a different IP than the one already added before s
...
(https://github.com/bitcoin/bitcoin/pull/31343)
Rather than opening a connection to a public IP, use one in the [link-local address range](https://en.wikipedia.org/wiki/Link-local_address#IPv4) in order to avoid the problems described in #31339. This is still not ideal and could be annoying to users if there is an actual machine reachable on this address, so I'm open for better suggestions. As far as I understand, we don't want to use localhost once again here, since for testing the filtering a different IP than the one already added before s
...