👍 rkrux approved a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2134800855)
tACK [72b2268](https://github.com/bitcoin/bitcoin/pull/30309/commits/72b226882fe2348a9a66aee1d8d21b4e2d275e68)
Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!
> Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight
I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in `coinselection`:
- https://githu
...
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2134800855)
tACK [72b2268](https://github.com/bitcoin/bitcoin/pull/30309/commits/72b226882fe2348a9a66aee1d8d21b4e2d275e68)
Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!
> Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight
I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in `coinselection`:
- https://githu
...
💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1650435604)
Any specific reason to generate 1472 outputs when 1471 are picked?
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1650435604)
Any specific reason to generate 1472 outputs when 1471 are picked?
💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1650434123)
Thanks for adding a functional test for `send()` as well.
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1650434123)
Thanks for adding a functional test for `send()` as well.
👍 rkrux approved a pull request: "[test]: prevent `create_self_transfer` failure when target weight is below tx weight"
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2134883494)
reACK [7d5afbb](https://github.com/bitcoin/bitcoin/pull/30322/commits/7d5afbb5627d754af5db3a2bd4beb5cf0f400b2c)
@ismaelsadeeq Thanks for quickly addressing my comments, these multiple small commits helped in thorough review of the PR.
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2134883494)
reACK [7d5afbb](https://github.com/bitcoin/bitcoin/pull/30322/commits/7d5afbb5627d754af5db3a2bd4beb5cf0f400b2c)
@ismaelsadeeq Thanks for quickly addressing my comments, these multiple small commits helped in thorough review of the PR.
👍 willcl-ark approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2135153143)
ACK a2fc9ddee9cbaeffb3c460973bab3c2dd734db55
Compared changes since last review using `git range-diff 8135632...a2fc9ddee9cbaeffb3c460973bab3c2dd734db55`, which added the `difflib` helper and updated the commit message.
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2135153143)
ACK a2fc9ddee9cbaeffb3c460973bab3c2dd734db55
Compared changes since last review using `git range-diff 8135632...a2fc9ddee9cbaeffb3c460973bab3c2dd734db55`, which added the `difflib` helper and updated the commit message.
📝 paplorinc converted_to_draft a pull request: "optimization: Switch CTxMemPool::CalculateDescendants from set to vector to reduce transaction hash calculations"
(https://github.com/bitcoin/bitcoin/pull/30325)
Profiling `BlockAssemblerAddPackageTxns` indicated excessive hash recalculations:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/e1687162-30b5-47b6-83b3-d234cd156395">
The original implementation used a set to manage the stack of transactions for a depth-first traversal of its child transactions. However, since we're already filtering via `setDescendants` before adding them to `stage`, duplicates cannot exist anyway. Therefore, we can change it to a simple vector, providing effi
...
(https://github.com/bitcoin/bitcoin/pull/30325)
Profiling `BlockAssemblerAddPackageTxns` indicated excessive hash recalculations:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/e1687162-30b5-47b6-83b3-d234cd156395">
The original implementation used a set to manage the stack of transactions for a depth-first traversal of its child transactions. However, since we're already filtering via `setDescendants` before adding them to `stage`, duplicates cannot exist anyway. Therefore, we can change it to a simple vector, providing effi
...
👋 paplorinc's pull request is ready for review: "optimization: Switch CTxMemPool::CalculateDescendants from set to vector to reduce transaction hash calculations"
(https://github.com/bitcoin/bitcoin/pull/30325)
(https://github.com/bitcoin/bitcoin/pull/30325)
👋 paplorinc's pull request is ready for review: "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin"
(https://github.com/bitcoin/bitcoin/pull/30326)
(https://github.com/bitcoin/bitcoin/pull/30326)
💬 rkrux commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2186203543)
> tACK [72b2268](https://github.com/bitcoin/bitcoin/pull/30309/commits/72b226882fe2348a9a66aee1d8d21b4e2d275e68)
>
> Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!
>
> > Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight
>
> I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in `coinselection`:
>
...
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2186203543)
> tACK [72b2268](https://github.com/bitcoin/bitcoin/pull/30309/commits/72b226882fe2348a9a66aee1d8d21b4e2d275e68)
>
> Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!
>
> > Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight
>
> I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in `coinselection`:
>
...
👋 fanquake's pull request is ready for review: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/30305)
(https://github.com/bitcoin/bitcoin/pull/30305)
👍 willcl-ark approved a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/30305#pullrequestreview-2135426523)
ACK b3093eb755c00f0dc6c71d8384c9078603ef4862
Manually compared each backported commit with its original version.
(https://github.com/bitcoin/bitcoin/pull/30305#pullrequestreview-2135426523)
ACK b3093eb755c00f0dc6c71d8384c9078603ef4862
Manually compared each backported commit with its original version.
👍 stickies-v approved a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/30305#pullrequestreview-2135517444)
ACK b3093eb755c00f0dc6c71d8384c9078603ef4862
All backports are clean, except:
- 0d524b14847f94d6e19413fde27f12e39e34694c backported from 9eea51d9058ad638861aa4b94c1c6e71caeb8765: `win64-native` CI steps `Clone fuzz corpus` and `RUun fuzz binaries` aren't backported, they are part of #29774
(https://github.com/bitcoin/bitcoin/pull/30305#pullrequestreview-2135517444)
ACK b3093eb755c00f0dc6c71d8384c9078603ef4862
All backports are clean, except:
- 0d524b14847f94d6e19413fde27f12e39e34694c backported from 9eea51d9058ad638861aa4b94c1c6e71caeb8765: `win64-native` CI steps `Clone fuzz corpus` and `RUun fuzz binaries` aren't backported, they are part of #29774
💬 stickies-v commented on pull request "[27.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650871619)
I know we usually do it this way, but what's the harm in just making this (and other references, except for the git tag) `27.2rc1` right away?
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650871619)
I know we usually do it this way, but what's the harm in just making this (and other references, except for the git tag) `27.2rc1` right away?
💬 fanquake commented on pull request "[27.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650873155)
The next release might not be 27.2. It could be 27.1.1 etc.
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650873155)
The next release might not be 27.2. It could be 27.1.1 etc.
💬 hebasto commented on issue "automake script error building 32 bit depends libevent-2.1.12":
(https://github.com/bitcoin/bitcoin/issues/30311#issuecomment-2186368722)
@techy2
> taking a look at focal32 expected path for cc1, it is not there. Installed gcc 10.5.0 once again and succeeded in populating /usr/lib/gcc/i686-linux-gnu/10/cc1 works now on focal32
>
> However the failure to cross compile on the x64 platform is an issue
>
> --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu from the config log does not look right to me for an 1686 cross build
Do I understand correctly that you has resolved the initial issue mentione
...
(https://github.com/bitcoin/bitcoin/issues/30311#issuecomment-2186368722)
@techy2
> taking a look at focal32 expected path for cc1, it is not there. Installed gcc 10.5.0 once again and succeeded in populating /usr/lib/gcc/i686-linux-gnu/10/cc1 works now on focal32
>
> However the failure to cross compile on the x64 platform is an issue
>
> --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu from the config log does not look right to me for an 1686 cross build
Do I understand correctly that you has resolved the initial issue mentione
...
💬 stickies-v commented on pull request "[27.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650897241)
Sure, but that seems to be an undocumented exception? Since 0.10.0, it [seems](https://bitcoincore.org/bin/) we only have 3 (in the root dir) releases where we we had a non-zero patch number. We also don't mention patch versions in our [documentation](https://bitcoincore.org/en/lifecycle/).
Keeping the git tag to `.x` makes sense because it gives us the flexibility to release as a minor or a patch version, i.e. we can still easily update the release notes to `27.1.1` if necessary. It just see
...
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650897241)
Sure, but that seems to be an undocumented exception? Since 0.10.0, it [seems](https://bitcoincore.org/bin/) we only have 3 (in the root dir) releases where we we had a non-zero patch number. We also don't mention patch versions in our [documentation](https://bitcoincore.org/en/lifecycle/).
Keeping the git tag to `.x` makes sense because it gives us the flexibility to release as a minor or a patch version, i.e. we can still easily update the release notes to `27.1.1` if necessary. It just see
...
💬 fanquake commented on pull request "[27.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650958819)
Ok. Lets follow up with ways we might want to streamline this for `28.x`.
(https://github.com/bitcoin/bitcoin/pull/30305#discussion_r1650958819)
Ok. Lets follow up with ways we might want to streamline this for `28.x`.
👍 rkrux approved a pull request: "Wallet: Add `max_tx_weight` to transaction funding options (take 2)"
(https://github.com/bitcoin/bitcoin/pull/29523#pullrequestreview-2135369361)
Concept ACK [723a8da](https://github.com/bitcoin/bitcoin/pull/29523/commits/723a8da8e562451facc8daa5d6ba61977f68cafe), but I would look at [this](https://github.com/bitcoin/bitcoin/pull/29523/commits/723a8da8e562451facc8daa5d6ba61977f68cafe) commit a bit more again; `make, test/functional` are successful though.
Following transaction funding RPCs are affected with the addition of `max_tx_weight` option:
1. `fundrawtransaction`
2. `walletcreatefundedpsbt`
3. `send`
(https://github.com/bitcoin/bitcoin/pull/29523#pullrequestreview-2135369361)
Concept ACK [723a8da](https://github.com/bitcoin/bitcoin/pull/29523/commits/723a8da8e562451facc8daa5d6ba61977f68cafe), but I would look at [this](https://github.com/bitcoin/bitcoin/pull/29523/commits/723a8da8e562451facc8daa5d6ba61977f68cafe) commit a bit more again; `make, test/functional` are successful though.
Following transaction funding RPCs are affected with the addition of `max_tx_weight` option:
1. `fundrawtransaction`
2. `walletcreatefundedpsbt`
3. `send`
💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1650773911)
As per the following 2 statements in `ChooseSelectionResult`:
- https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L699
- https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L709
The `no_input` and `change_output` weights are accounted for, but where is the weight of the destination output accounted for?
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1650773911)
As per the following 2 statements in `ChooseSelectionResult`:
- https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L699
- https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L709
The `no_input` and `change_output` weights are accounted for, but where is the weight of the destination output accounted for?
💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1650938860)
Was the intention to write "In order to ..."?
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1650938860)
Was the intention to write "In order to ..."?