🚀 ryanofsky merged a pull request: "net: Block v2->v1 transport downgrade if !fNetworkActive"
(https://github.com/bitcoin/bitcoin/pull/32073)
(https://github.com/bitcoin/bitcoin/pull/32073)
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749384672)
@ryanofsky, thanks again for the review.
Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits, where we didn't know the full size of the block, and read and wrote in chunks.
This current alternative was explicitly suggested by @theuni - and while it's a bit messy to always read the size, the resulting code is faster and seem to have fewer moving parts (e.g. doesn't reassign via the parameter, doesn't copy the w
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749384672)
@ryanofsky, thanks again for the review.
Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits, where we didn't know the full size of the block, and read and wrote in chunks.
This current alternative was explicitly suggested by @theuni - and while it's a bit messy to always read the size, the resulting code is faster and seem to have fewer moving parts (e.g. doesn't reassign via the parameter, doesn't copy the w
...
💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010928029)
For segwitv0, program sizes other than 20 or 32 are indeed consensus-invalid (see function [VerifyWitnessProgram](https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/src/script/interpreter.cpp#L1900-L1902)), so the comment is correct. And yes, for segwitv1+, not-yet-defined programs are only non-standard, but consensus-valid:
https://github.com/bitcoin/bitcoin/blob/a0d737cd7a7fafe2c743360c4ac7b2e72664e5c5/src/script/interpreter.cpp#L1952-L1953
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010928029)
For segwitv0, program sizes other than 20 or 32 are indeed consensus-invalid (see function [VerifyWitnessProgram](https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/src/script/interpreter.cpp#L1900-L1902)), so the comment is correct. And yes, for segwitv1+, not-yet-defined programs are only non-standard, but consensus-valid:
https://github.com/bitcoin/bitcoin/blob/a0d737cd7a7fafe2c743360c4ac7b2e72664e5c5/src/script/interpreter.cpp#L1952-L1953
💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010929617)
Will apply if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010929617)
Will apply if I have to retouch.
💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010931145)
Could this be part of #32096?
Another part is probably better to change at least as early:
https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/doc/bitcoin-conf.md?plain=1#L27
Probably better to use "regtest=1" or "signet=1".
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010931145)
Could this be part of #32096?
Another part is probably better to change at least as early:
https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/doc/bitcoin-conf.md?plain=1#L27
Probably better to use "regtest=1" or "signet=1".
💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010933670)
Noticed while looking at #32096 that this file was untouched there.
Testnet3 has ***rpc***port=18332 according to how chainparamsbase.cpp sets `CBaseChainParams(..., rpc_port)`...
This config is setting the P2P ports for all networks so that they will collide with the default RPC ports and prevent successful startup (verified with the mainnet config line). Might as well just remove all ports IMO.
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010933670)
Noticed while looking at #32096 that this file was untouched there.
Testnet3 has ***rpc***port=18332 according to how chainparamsbase.cpp sets `CBaseChainParams(..., rpc_port)`...
This config is setting the P2P ports for all networks so that they will collide with the default RPC ports and prevent successful startup (verified with the mainnet config line). Might as well just remove all ports IMO.
💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749457567)
> @ryanofsky, thanks again for the review. Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits
Absolutely yes. There are a few things I would change about that PR but overall it seems a lot cleaner and simpler to me than this PR. If it doesn't perform as well as this one that is surprising, because I thought the performance improvement here came from buffering, not from choosing buffers with magic sizes. But
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749457567)
> @ryanofsky, thanks again for the review. Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits
Absolutely yes. There are a few things I would change about that PR but overall it seems a lot cleaner and simpler to me than this PR. If it doesn't perform as well as this one that is surprising, because I thought the performance improvement here came from buffering, not from choosing buffers with magic sizes. But
...
🚀 ryanofsky merged a pull request: "doc: clarify that testnet min-difficulty is not optional"
(https://github.com/bitcoin/bitcoin/pull/32095)
(https://github.com/bitcoin/bitcoin/pull/32095)
👍 hodlinator approved a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2711717129)
Code Review ACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
Adds basic tests for WitnessV1Taproot & (WitnessV1)PayToAnchor outputs.
Only non-test change is replacing array literal with `std::vector` (which sort of makes sense considering `struct PayToAnchor` (ab)uses `WitnessUnknown` as a base (predating this PR) which takes `vector` as a constructor input).
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2711717129)
Code Review ACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
Adds basic tests for WitnessV1Taproot & (WitnessV1)PayToAnchor outputs.
Only non-test change is replacing array literal with `std::vector` (which sort of makes sense considering `struct PayToAnchor` (ab)uses `WitnessUnknown` as a base (predating this PR) which takes `vector` as a constructor input).
💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010951768)
Thanks for the references! I'm still unfamiliar with that part. So for `MANDATORY_SCRIPT_VERIFY_FLAGS` we reach the return true there for Taproot. :+1:
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010951768)
Thanks for the references! I'm still unfamiliar with that part. So for `MANDATORY_SCRIPT_VERIFY_FLAGS` we reach the return true there for Taproot. :+1:
💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2749480675)
Rebased 26c2136a2bc7fec9697f61eadf422c439e0735a2 -> c9a49633a027f1c28ebe15c0134f1ce38b712e8a ([`pr/ipc.216`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.216) -> [`pr/ipc.217`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.217), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc.216-rebase..pr/ipc.217)) due to silent conflict with #31519
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2749480675)
Rebased 26c2136a2bc7fec9697f61eadf422c439e0735a2 -> c9a49633a027f1c28ebe15c0134f1ce38b712e8a ([`pr/ipc.216`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.216) -> [`pr/ipc.217`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.217), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc.216-rebase..pr/ipc.217)) due to silent conflict with #31519
💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010972739)
Well if the asmap is reasonably up to date, I suspect there will basically be ~no peers in unmapped portions. And if there are, if unfilled it'll use /16 grouping, and if filled it'll use an adjacent ASN. I couldn't say which would be better in general among those.
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010972739)
Well if the asmap is reasonably up to date, I suspect there will basically be ~no peers in unmapped portions. And if there are, if unfilled it'll use /16 grouping, and if filled it'll use an adjacent ASN. I couldn't say which would be better in general among those.
💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010999772)
Thanks for checking out the demo!
For the naming, I tried to use names similar to BIP-345, except that I name the "Vault" and "Unvaulting" state of the FSM which seems more natural in CCV-lingo (where you define the multi-utxo contract as a finite-state-machine).
> Can `alternate_pk` and `recover_pk` just be the same? That way there's only two key sets needed for a vault: one hot, one cold. And you're avoiding the use of a NUMS point.
You could, but I don't think that makes a lot of sense
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010999772)
Thanks for checking out the demo!
For the naming, I tried to use names similar to BIP-345, except that I name the "Vault" and "Unvaulting" state of the FSM which seems more natural in CCV-lingo (where you define the multi-utxo contract as a finite-state-machine).
> Can `alternate_pk` and `recover_pk` just be the same? That way there's only two key sets needed for a vault: one hot, one cold. And you're avoiding the use of a NUMS point.
You could, but I don't think that makes a lot of sense
...
💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010985584)
Why not move this to #32096? (23607f605c3156ca904f704666f7cbe4808cf76b there touches the entry from the vaguely related `MAGIC_BYTES`).
Why not have a "testnet4" entry?
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010985584)
Why not move this to #32096? (23607f605c3156ca904f704666f7cbe4808cf76b there touches the entry from the vaguely related `MAGIC_BYTES`).
Why not have a "testnet4" entry?
💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011041024)
(Inline thread in random position)
nit: src/external_signer.h references "test" and never "testnet4" which seems to be the argument the code would be sending in.
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011041024)
(Inline thread in random position)
nit: src/external_signer.h references "test" and never "testnet4" which seems to be the argument the code would be sending in.
👍 hodlinator approved a pull request: "Move some tests and documentation from testnet3 to testnet4"
(https://github.com/bitcoin/bitcoin/pull/32096#pullrequestreview-2711851156)
ACK 2906b183169bc78b37449a818717249c2d1cb7a1
Ran functional tests with `--extended` with on a non-multiprocess `devmode` build (in part to verify network `MAGIC_BYTES` change).
(https://github.com/bitcoin/bitcoin/pull/32096#pullrequestreview-2711851156)
ACK 2906b183169bc78b37449a818717249c2d1cb7a1
Ran functional tests with `--extended` with on a non-multiprocess `devmode` build (in part to verify network `MAGIC_BYTES` change).
💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011028887)
Should be removed as these tests currently duplicate the L669-670. Don't think those checks mean to be dependent on surrounding lines. Probably good to keep L685+L687 though.
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011028887)
Should be removed as these tests currently duplicate the L669-670. Don't think those checks mean to be dependent on surrounding lines. Probably good to keep L685+L687 though.
💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011039690)
(Inline thread in random position)
informational: https://developer.bitcoin.org/examples/intro.html / https://github.com/bitcoin-dot-org/developer.bitcoin.org/blob/master/examples/intro.rst
still references the Testnet3 RPC port 18332. But the repo hasn't been updated for 4 years.
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011039690)
(Inline thread in random position)
informational: https://developer.bitcoin.org/examples/intro.html / https://github.com/bitcoin-dot-org/developer.bitcoin.org/blob/master/examples/intro.rst
still references the Testnet3 RPC port 18332. But the repo hasn't been updated for 4 years.
💬 chellas2wp commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2011428282)
Starting process/approval request/apply specifications.
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2011428282)
Starting process/approval request/apply specifications.
💬 davidgumberg commented on issue "wallet: migratewallet crashes "Assertion `legacy_spkm' failed"":
(https://github.com/bitcoin/bitcoin/issues/32112#issuecomment-2750254947)
Just to add context, the crash is on an [`Assume()`](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4087) so it can't happen in a production build, where an unhelpful [error message is printed](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4089) when this happens.
This should be caught by `BerkeleyRODatabase::Open()` which makes [efforts](https://github.com/bitcoin/bitcoin/blob/1
...
(https://github.com/bitcoin/bitcoin/issues/32112#issuecomment-2750254947)
Just to add context, the crash is on an [`Assume()`](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4087) so it can't happen in a production build, where an unhelpful [error message is printed](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4089) when this happens.
This should be caught by `BerkeleyRODatabase::Open()` which makes [efforts](https://github.com/bitcoin/bitcoin/blob/1
...