💬 TheCharlatan commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1933573379)
I think there is room for the functional tests to demonstrate some "common misconfigurations" to help avoid regressions, but agree that this case here is probably best documented and described elsewhere.
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1933573379)
I think there is room for the functional tests to demonstrate some "common misconfigurations" to help avoid regressions, but agree that this case here is probably best documented and described elsewhere.
💬 Sjors commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1933586381)
It's in https://github.com/bitcoin/bitcoin/pull/31725
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1933586381)
It's in https://github.com/bitcoin/bitcoin/pull/31725
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516)
01452f8c0da58db549bc46d0cfa7de715344efc4: the Apple version of tar doesn't have `--sort`, see https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516)
01452f8c0da58db549bc46d0cfa7de715344efc4: the Apple version of tar doesn't have `--sort`, see https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394
👍 TheCharlatan approved a pull request: "depends: Update libmultiprocess library before converting to subtree"
(https://github.com/bitcoin/bitcoin/pull/31740#pullrequestreview-2580718512)
ACK 4e0aa1835b3e980ceda29ec90e7115d7fef53f51
(https://github.com/bitcoin/bitcoin/pull/31740#pullrequestreview-2580718512)
ACK 4e0aa1835b3e980ceda29ec90e7115d7fef53f51
🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2580366855)
Concept ACK fefe0636d4ae7c246042276cacd60b22f5fc6bb9
Good PR with enough context to unpack for me. Left few comments, will review again soon.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2580366855)
Concept ACK fefe0636d4ae7c246042276cacd60b22f5fc6bb9
Good PR with enough context to unpack for me. Left few comments, will review again soon.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933484246)
`If only the parameter is provided`
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933484246)
`If only the parameter is provided`
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933559100)
`std::optional<int> sighash_type = 1 /* SIGHASH_ALL */`
Does it not require `std::nullopt` as the default like in other cases?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933559100)
`std::optional<int> sighash_type = 1 /* SIGHASH_ALL */`
Does it not require `std::nullopt` as the default like in other cases?
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933618280)
Confirming - it's being removed from here because this check now happens inside `SignPSBTInput` that is called down below?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933618280)
Confirming - it's being removed from here because this check now happens inside `SignPSBTInput` that is called down below?
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933698058)
Can this commit be a separate PR? It would be nice to have a functional test that checks non witness utxos are dropped if sighash type is not `SIGHASH_ANYONECANPAY`. It can be added in this PR as well at the cost of a larger diff.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933698058)
Can this commit be a separate PR? It would be nice to have a functional test that checks non witness utxos are dropped if sighash type is not `SIGHASH_ANYONECANPAY`. It can be added in this PR as well at the cost of a larger diff.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933483132)
`parameter`
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933483132)
`parameter`
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933604449)
`If only the psbt field is provided, refuse to sign.`
How does it refuse to sign in this case?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933604449)
`If only the psbt field is provided, refuse to sign.`
How does it refuse to sign in this case?
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933734099)
Nit feel free to ignore: I ignored this duplication in the first commit but now that it's being done twice, a refactor could be to just iterate this in a loop of sighash types.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933734099)
Nit feel free to ignore: I ignored this duplication in the first commit but now that it's being done twice, a refactor could be to just iterate this in a loop of sighash types.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933731594)
Atm, there is only 1 input in the PSBT. It would be nice to have another input that is Taproot and check for its sighash.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933731594)
Atm, there is only 1 input in the PSBT. It would be nice to have another input that is Taproot and check for its sighash.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933654773)
Is the downside of adding sighash in PSBT in case of default or all is that it increases PSBT size?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933654773)
Is the downside of adding sighash in PSBT in case of default or all is that it increases PSBT size?
💬 TheCharlatan commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621465199)
As an alternative to reaching into depends or committing generated files, could it be feasible to set native compilers in the toolchain file instead for compiling the code generators?
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621465199)
As an alternative to reaching into depends or committing generated files, could it be feasible to set native compilers in the toolchain file instead for compiling the code generators?
🚀 fanquake merged a pull request: "depends: Update libmultiprocess library before converting to subtree"
(https://github.com/bitcoin/bitcoin/pull/31740)
(https://github.com/bitcoin/bitcoin/pull/31740)
🤔 glozow reviewed a pull request: "test: fix intermittent timeout in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/pull/31751#pullrequestreview-2580850613)
utACK 215d8b0a2ccf3a7275ea483339c58da5e0085e83, the explanation makes sense to me and agree the p2ps don't need to be connected after this point.
(https://github.com/bitcoin/bitcoin/pull/31751#pullrequestreview-2580850613)
utACK 215d8b0a2ccf3a7275ea483339c58da5e0085e83, the explanation makes sense to me and agree the p2ps don't need to be connected after this point.
💬 hebasto commented on issue "build: `cmake --install` fails if only select targets are built":
(https://github.com/bitcoin/bitcoin/issues/31745#issuecomment-2621498472)
> I think what you're really looking for is a way to only build/install certain binaries. For that, the workaround would be to name each one as a separate component. That would allow for:
>
> cmake -B build
> cmake --build build --target bitcoind bitcoin-cli
> cmake --install build --component bitcoind bitcoin-cli
I agree. We have already [done](https://github.com/bitcoin/bitcoin/pull/30835) it for the `bitcoinkernel` target.
On the other hand, the [`CMAKE_SKIP_INSTALL_ALL_DEPENDENCY`](https:
...
(https://github.com/bitcoin/bitcoin/issues/31745#issuecomment-2621498472)
> I think what you're really looking for is a way to only build/install certain binaries. For that, the workaround would be to name each one as a separate component. That would allow for:
>
> cmake -B build
> cmake --build build --target bitcoind bitcoin-cli
> cmake --install build --component bitcoind bitcoin-cli
I agree. We have already [done](https://github.com/bitcoin/bitcoin/pull/30835) it for the `bitcoinkernel` target.
On the other hand, the [`CMAKE_SKIP_INSTALL_ALL_DEPENDENCY`](https:
...
🤔 hebasto reviewed a pull request: "doc: update links in ci.yml"
(https://github.com/bitcoin/bitcoin/pull/31736#pullrequestreview-2580887381)
Post-merge ACK 1681c08d42c302088586ca9b36b278217723f66a.
(https://github.com/bitcoin/bitcoin/pull/31736#pullrequestreview-2580887381)
Post-merge ACK 1681c08d42c302088586ca9b36b278217723f66a.
💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621519226)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621519226)
Concept ACK.