Bitcoin Core Github
43 subscribers
122K links
Download Telegram
🤔 instagibbs reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2451932627)
how about

```
diff --git a/src/addresstype.h b/src/addresstype.h
index 93cdf66c5b..09f3063c61 100644
--- a/src/addresstype.h
+++ b/src/addresstype.h
@@ -115,14 +115,17 @@ public:
if (w1.GetWitnessVersion() > w2.GetWitnessVersion()) return false;
return w1.GetWitnessProgram() < w2.GetWitnessProgram();
}
};

+/** Witness program for Pay-to-Anchor output script type */
+static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
+
struct PayToAnchor
...
💬 instagibbs commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1852412865)
nit: this would be "undefined" rather than "incorrect" like the segwitv0 case
💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1852555732)
Reworked the comments to match the pattern used in previous cases, which is
`// TxoutType::INTENDED_TYPE with <some minor change that leads to different solver outcome>`
(the comment _"WITNESS_UNKNOWN with incorrect program size"_ didn't make any sense imho)

I kept the "incorrect" term, but wrote the outcome explanation in parantheses. Open for suggestions.
💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#issuecomment-2491828622)

> how about
> ```
> ...
> ```
> which should get rid of all instances of 0x4e, 0x73?

@instagibbs: Thanks, taken!

> Could also move it into script/script.h and make it a static member of CScript?

Kept as-is for minimal diff, but happy to also do that change if others feel strongly about it.
💬 instagibbs commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#issuecomment-2491852015)
ACK https://github.com/bitcoin/bitcoin/pull/31340/commits/a3a4d199e298a76725d0c0424195ae54c7e95fe0
💬 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
💬 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
🤔 BrandonOdiwuor reviewed a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(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
🤔 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.
💬 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).
💬 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)
📝 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.
-->
willcl-ark closed a pull request: "Add files via upload"
(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.
-->
💬 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://
...
👍 TheCharlatan approved a pull request: "Drop script_pub_key arg from createNewBlock"
(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.
👍 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.
🤔 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
...