🚀 achow101 merged a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909)
(https://github.com/bitcoin/bitcoin/pull/30909)
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628373528)
> Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see [NixOS/nixpkgs#359902](https://github.com/NixOS/nixpkgs/issues/359902)) that are more flexible and less monolithic than the depends system.
Perhaps this is the source of our breakdown. I would consider this an anti-feature. Or AT LEAST, as you've be
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628373528)
> Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see [NixOS/nixpkgs#359902](https://github.com/NixOS/nixpkgs/issues/359902)) that are more flexible and less monolithic than the depends system.
Perhaps this is the source of our breakdown. I would consider this an anti-feature. Or AT LEAST, as you've be
...
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628379396)
> Only an extra 2000 WU, right?
My understanding goes like this:
Imagine a miner that never needs more than 4000 for their coinbase, and has always used default settings. Their blocks have left behind 4000 empty space. After this patch, that same miner continuing to use default settings will continue to leave 4000 empty space per block. But now with the patch that miner has the option of filling that space with transaction data using `-blockreservedweight=4000`
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628379396)
> Only an extra 2000 WU, right?
My understanding goes like this:
Imagine a miner that never needs more than 4000 for their coinbase, and has always used default settings. Their blocks have left behind 4000 empty space. After this patch, that same miner continuing to use default settings will continue to leave 4000 empty space per block. But now with the patch that miner has the option of filling that space with transaction data using `-blockreservedweight=4000`
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937978920)
This unfortunately leaves us open to a supply-chain attack as it depends on the contents of the builders's environment. We'll need to commit to the hash of a deterministic tarball.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937978920)
This unfortunately leaves us open to a supply-chain attack as it depends on the contents of the builders's environment. We'll need to commit to the hash of a deterministic tarball.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628395523)
Thanks for your review, @pinheadmz and @fjahr.
I have answered the review questions and will address any nits when a retouch is needed.
@fjahr, think of it this way:
In the block assembler, we need to leave some space for the coinbase transaction, block header, and transaction count.
Previously, we reserved this space in multiple places `4000 WU` twice.
The `blockmaxweight` was described as `3996000 WU` because we intended to reserve space only once. However, due to duplicate
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628395523)
Thanks for your review, @pinheadmz and @fjahr.
I have answered the review questions and will address any nits when a retouch is needed.
@fjahr, think of it this way:
In the block assembler, we need to leave some space for the coinbase transaction, block header, and transaction count.
Previously, we reserved this space in multiple places `4000 WU` twice.
The `blockmaxweight` was described as `3996000 WU` because we intended to reserve space only once. However, due to duplicate
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628408316)
> I would consider this an anti-feature. Or AT LEAST, as you've been saying, one that can be added on top of the changes here once it's been discussed.
Well, if it is an anti-feature, it is a 5 line anti-feature and I am happy to drop it here. If I do this though I will probably make a separate PR to add it later, because I think it will be painful to develop and test upstream libmultiprocess changes without it.
> Anyway, I've pushed a demo of my changes here: https://github.com/theuni/bit
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628408316)
> I would consider this an anti-feature. Or AT LEAST, as you've been saying, one that can be added on top of the changes here once it's been discussed.
Well, if it is an anti-feature, it is a 5 line anti-feature and I am happy to drop it here. If I do this though I will probably make a separate PR to add it later, because I think it will be painful to develop and test upstream libmultiprocess changes without it.
> Anyway, I've pushed a demo of my changes here: https://github.com/theuni/bit
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933929575)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516
> [01452f8](https://github.com/bitcoin/bitcoin/commit/01452f8c0da58db549bc46d0cfa7de715344efc4): the Apple version of tar doesn't have `--sort`, see [chaincodelabs/libmultiprocess#139 (comment)](https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394)
Thanks, this is dropped
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933929575)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933669516
> [01452f8](https://github.com/bitcoin/bitcoin/commit/01452f8c0da58db549bc46d0cfa7de715344efc4): the Apple version of tar doesn't have `--sort`, see [chaincodelabs/libmultiprocess#139 (comment)](https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621080394)
Thanks, this is dropped
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933926424)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1931807802
Thanks I reworked this and adding headings to separate the cases. I think base case here is that capnproto is the only external dependency and you install it with package manager on your system. A second comon option may be to use depends, so that is described second. The third option of installing libmultiprocess externally is last because I was thinking that might be less commonly done, but I'm not sure.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1933926424)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1931807802
Thanks I reworked this and adding headings to separate the cases. I think base case here is that capnproto is the only external dependency and you install it with package manager on your system. A second comon option may be to use depends, so that is described second. The third option of installing libmultiprocess externally is last because I was thinking that might be less commonly done, but I'm not sure.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937988473)
I am not following. Can you describe the concern?
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1937988473)
I am not following. Can you describe the concern?
💬 achow101 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_r1937998551)
It's for compatibility, particularly when signing a transaction that has both taproot and non-taproot inputs.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1937998551)
It's for compatibility, particularly when signing a transaction that has both taproot and non-taproot inputs.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628425499)
@ismaelsadeeq @pinheadmz Ok, I see what you are trying to say. But if we are assuming the miner was smart and using the config options available to optimize, then they could have already reached `3996000 WU` by setting `blockmaxweight` to `4000000 WU` before this change, right? This is even mentioned in the release notes... Maybe the docs make it more realistic that miners actually do this but the comment was about behavior change, not the docs. So I think the actual delta made possible by the b
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628425499)
@ismaelsadeeq @pinheadmz Ok, I see what you are trying to say. But if we are assuming the miner was smart and using the config options available to optimize, then they could have already reached `3996000 WU` by setting `blockmaxweight` to `4000000 WU` before this change, right? This is even mentioned in the release notes... Maybe the docs make it more realistic that miners actually do this but the comment was about behavior change, not the docs. So I think the actual delta made possible by the b
...
💬 achow101 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_r1938002644)
I don't think that's actually correct either as then passing SIGHASH_DEFAULT will be unable to sign for non-taproot at the same time.
It's also a much larger change that I think is out of scope for this PR.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938002644)
I don't think that's actually correct either as then passing SIGHASH_DEFAULT will be unable to sign for non-taproot at the same time.
It's also a much larger change that I think is out of scope for this PR.
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628435688)
@ryanofsky Thanks.
I think we might just have been disagreeing on the importance of a unified build for everyone for something that will presumably (in the future) become mission-critical. I can see how from your POV it might seems like I'm just being persnickety about flags, but to me it's about making sure we're being clear (internally and externally) about ownership/canon/etc... the flags were just illustrative (to me) of the lack of clarity there.
Thanks for your patience with me on th
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628435688)
@ryanofsky Thanks.
I think we might just have been disagreeing on the importance of a unified build for everyone for something that will presumably (in the future) become mission-critical. I can see how from your POV it might seems like I'm just being persnickety about flags, but to me it's about making sure we're being clear (internally and externally) about ownership/canon/etc... the flags were just illustrative (to me) of the lack of clarity there.
Thanks for your patience with me on th
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1938013882)
> I am not following. Can you describe the concern?
I would think if anything we are more vulnerable to supply-chain attack **without** this change, because, for example if a few bitcoin core developers conspired and made a version of libmultiprocess with a backdoor and pushed it to github, it wouldn't be obvious to other developers that the hashes pointed to a backdoor version. It also wouldn't be obvious to external observers diffing one version of bitcoin to the next to check for introduct
...
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1938013882)
> I am not following. Can you describe the concern?
I would think if anything we are more vulnerable to supply-chain attack **without** this change, because, for example if a few bitcoin core developers conspired and made a version of libmultiprocess with a backdoor and pushed it to github, it wouldn't be obvious to other developers that the hashes pointed to a backdoor version. It also wouldn't be obvious to external observers diffing one version of bitcoin to the next to check for introduct
...
💬 davidgumberg commented on issue "crypto: secure erase memory":
(https://github.com/bitcoin/bitcoin/issues/31744#issuecomment-2628450376)
Thanks for reporting, I've opened #31774 to address this.
(https://github.com/bitcoin/bitcoin/issues/31744#issuecomment-2628450376)
Thanks for reporting, I've opened #31774 to address this.
💬 achow101 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_r1938014929)
Yes
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938014929)
Yes
💬 achow101 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_r1938015888)
Yes. Additionally, specifying `ALL` for taproot means adding another byte to the signature and also changes the sighash.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938015888)
Yes. Additionally, specifying `ALL` for taproot means adding another byte to the signature and also changes the sighash.
💬 achow101 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_r1938016937)
No, this was added because without it there were test failures.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938016937)
No, this was added because without it there were test failures.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628463063)
> disagreeing on the importance of a unified build for everyone for something that will presumably (in the future) become mission-critical.
I really don't think this is true. I think you have made number a lot of claims that sound great, don't stand up to scrutiny. I probably have too. These issues can be hard to reason about, so it's good that we are going through this exercise.
But I don't take security or hardening less seriously than anyone else, and I believe this approach has been 10
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628463063)
> disagreeing on the importance of a unified build for everyone for something that will presumably (in the future) become mission-critical.
I really don't think this is true. I think you have made number a lot of claims that sound great, don't stand up to scrutiny. I probably have too. These issues can be hard to reason about, so it's good that we are going through this exercise.
But I don't take security or hardening less seriously than anyone else, and I believe this approach has been 10
...
💬 stickies-v commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2628469196)
Thank you for your effort in approaching this issue in a structural way, @ryanofsky . I'm not advocating for or against shipping multiprocess in 29.0 - I could be okay with either, I haven't reviewed the code enough to have an opinion on that yet. My concerns quite closely align with [darosior's write-up](https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622618673). It appears more review and dog-fooding could be helpful, and I just haven't seen an urgent reason to ship this with 29.
...
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2628469196)
Thank you for your effort in approaching this issue in a structural way, @ryanofsky . I'm not advocating for or against shipping multiprocess in 29.0 - I could be okay with either, I haven't reviewed the code enough to have an opinion on that yet. My concerns quite closely align with [darosior's write-up](https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622618673). It appears more review and dog-fooding could be helpful, and I just haven't seen an urgent reason to ship this with 29.
...