💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483090)
Agree, leaving that as a follow-up as it likely needs more instances to adapt that are not touched here.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483090)
Agree, leaving that as a follow-up as it likely needs more instances to adapt that are not touched here.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483152)
> I've been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?
Yes, exactly. The encoded length of the unpadded output script fits into one byte, and we want to find how much the length encoding increases with the padding, so we subtract by one. I added a comment to (hopefully) reduce the confusion for code readers.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483152)
> I've been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?
Yes, exactly. The encoded length of the unpadded output script fits into one byte, and we want to find how much the length encoding increases with the padding, so we subtract by one. I added a comment to (hopefully) reduce the confusion for code readers.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483290)
> Why are you incrementing by 3 twice
The first increment by 3 is done to find the worst-case weight (that could result from ._bulk_tx), the second is part of a weight-to-vsize conversion. I've changed the latter to use float division and rounding up, as that should reflect better what this calculation actually achieves (the same way it's done e.g. in `CTransaction.get_vsize()`). It might be worth to introduce a `weight_to_vsize` helper in the util module in a follow-up, which could probably
...
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483290)
> Why are you incrementing by 3 twice
The first increment by 3 is done to find the worst-case weight (that could result from ._bulk_tx), the second is part of a weight-to-vsize conversion. I've changed the latter to use float division and rounding up, as that should reflect better what this calculation actually achieves (the same way it's done e.g. in `CTransaction.get_vsize()`). It might be worth to introduce a `weight_to_vsize` helper in the util module in a follow-up, which could probably
...
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483453)
This sounds like a very good idea for a follow-up, as it's not directly related to this PR (though one could argue that it's more likely to happen with large `target_weight` values). I think it's even worth it to throw an exception with an explicit message here like "UTXO is too small to cover for fees" (maybe even including details like utxo value, absolute fee, fee-rate, vsize) rather than only an assertion on the send value, which could still be confusing for users.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621483453)
This sounds like a very good idea for a follow-up, as it's not directly related to this PR (though one could argue that it's more likely to happen with large `target_weight` values). I think it's even worth it to throw an exception with an explicit message here like "UTXO is too small to cover for fees" (maybe even including details like utxo value, absolute fee, fee-rate, vsize) rather than only an assertion on the send value, which could still be confusing for users.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2140953559)
@rkrux @ismaelsadeeq Thanks for your reviews, much appreciated! I tried to tackle the comments, though for some of them I suggested that they would be a better fit for a follow-up PR, to not increase the size this PR (focused on functional change) with too many refactoring commits that would likely make most sense with changes also in other functional tests and framework modules.
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2140953559)
@rkrux @ismaelsadeeq Thanks for your reviews, much appreciated! I tried to tackle the comments, though for some of them I suggested that they would be a better fit for a follow-up PR, to not increase the size this PR (focused on functional change) with too many refactoring commits that would likely make most sense with changes also in other functional tests and framework modules.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621491730)
One thing that's maybe worth mentioning here is that the method of padding is changed from "one push of N bytes" to "N single pushes" (using repeated OP_1 instructions). The former method could introduce additional bytes due to the length encoding of the pushed data (using either single-byte length bytes for sizes <= 75, and OP_PUSHDATA{1,2,4} for larger ones, see https://en.bitcoin.it/wiki/Script#Constants), making exact padding even harder, which is avoided with the new way.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621491730)
One thing that's maybe worth mentioning here is that the method of padding is changed from "one push of N bytes" to "N single pushes" (using repeated OP_1 instructions). The former method could introduce additional bytes due to the length encoding of the pushed data (using either single-byte length bytes for sizes <= 75, and OP_PUSHDATA{1,2,4} for larger ones, see https://en.bitcoin.it/wiki/Script#Constants), making exact padding even harder, which is avoided with the new way.
💬 fjahr commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2141013222)
tACK ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883
I reproduced the issue on current master with signet and confirmed that it is fixed with the change here (three times actually with slightly different logs). Based on reading the code and some custom logging I agree with @mzumsande that his description above is what is indeed happening. Based on this I am fine to add this without a test.
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2141013222)
tACK ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883
I reproduced the issue on current master with signet and confirmed that it is fixed with the change here (three times actually with slightly different logs). Based on reading the code and some custom logging I agree with @mzumsande that his description above is what is indeed happening. Based on this I am fine to add this without a test.
💬 fjahr commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2141014859)
@ryanofsky would be really great to get your review here when you get the chance :)
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2141014859)
@ryanofsky would be really great to get your review here when you get the chance :)
✅ achow101 closed a pull request: "Fix typos in 36 files | Almost only documentation"
(https://github.com/bitcoin/bitcoin/pull/30188)
(https://github.com/bitcoin/bitcoin/pull/30188)
💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1621719893)
I am somewhat ~0 on erroring in the case of 8manual/0in/0out, but my bigger anxiety with the latest commit is that it makes it confusing again (or too complicated in my judgement). I tried to code a simple way to account for the automatic outbounds but gave up. Also, most naturally, as the "minimum required" name suggests, the check should be `if (... < min_required)`. I find `if (... < min_required + something more)` confusing.
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1621719893)
I am somewhat ~0 on erroring in the case of 8manual/0in/0out, but my bigger anxiety with the latest commit is that it makes it confusing again (or too complicated in my judgement). I tried to code a simple way to account for the automatic outbounds but gave up. Also, most naturally, as the "minimum required" name suggests, the check should be `if (... < min_required)`. I find `if (... < min_required + something more)` confusing.
💬 vasild commented on pull request "net: log connections failures via SOCKS5 with less severity":
(https://github.com/bitcoin/bitcoin/pull/30064#issuecomment-2141242771)
`7524aaf58f...f3cfbd65f5`: drop the changes to `LogConnectFailure()`. They were somewhat related, but not directly to the SOCKS5 stuff.
> perhaps look if other ones there could be done here
The changes from #25203 look sound, but I want to keep this PR small, just to address the noisy and inconsistent logging due to connection failures via the SOCKS5 proxy.
(https://github.com/bitcoin/bitcoin/pull/30064#issuecomment-2141242771)
`7524aaf58f...f3cfbd65f5`: drop the changes to `LogConnectFailure()`. They were somewhat related, but not directly to the SOCKS5 stuff.
> perhaps look if other ones there could be done here
The changes from #25203 look sound, but I want to keep this PR small, just to address the noisy and inconsistent logging due to connection failures via the SOCKS5 proxy.
💬 vasild commented on pull request "net: log connections failures via SOCKS5 with less severity":
(https://github.com/bitcoin/bitcoin/pull/30064#discussion_r1621727838)
Dropped this hunk altogether.
(https://github.com/bitcoin/bitcoin/pull/30064#discussion_r1621727838)
Dropped this hunk altogether.
💬 maflcko commented on pull request "depends: qt 5.15.14 and fix macOS build with Clang 18":
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2141246562)
Tested on:
```
# clang --version && file src/qt/bitcoin-qt
Ubuntu clang version 18.1.3 (1)
Target: riscv64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
src/qt/bitcoin-qt: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|WEAK_DEFINES|BINDS_TO_WEAK|PIE|HAS_TLV_DESCRIPTORS>
```
```
# clang --version && file src/qt/bitcoin-qt
Ubuntu clang version 18.1.3 (1)
Target: powerpc64le-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
src/qt/bit
...
(https://github.com/bitcoin/bitcoin/pull/30198#issuecomment-2141246562)
Tested on:
```
# clang --version && file src/qt/bitcoin-qt
Ubuntu clang version 18.1.3 (1)
Target: riscv64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
src/qt/bitcoin-qt: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|WEAK_DEFINES|BINDS_TO_WEAK|PIE|HAS_TLV_DESCRIPTORS>
```
```
# clang --version && file src/qt/bitcoin-qt
Ubuntu clang version 18.1.3 (1)
Target: powerpc64le-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
src/qt/bit
...
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621795298)
Thanks for the confirmation, appreciate it!
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621795298)
Thanks for the confirmation, appreciate it!
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621797908)
> To my knowledge, weight is always measured in weight units, so I don't think adding the _wu suffix here would add any new information.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621797908)
> To my knowledge, weight is always measured in weight units, so I don't think adding the _wu suffix here would add any new information.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621812773)
Agreed. However if it had not been for the absolute fee, we could come up with a more generic function `get_effective_unspent_value` that calculates based on the `fee_rate` and the `vsize` of the unspent.
Can't include the absolute fee in this because it is paid for the whole transaction and not just one unspent - still valid for transactions spending only one unspent.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621812773)
Agreed. However if it had not been for the absolute fee, we could come up with a more generic function `get_effective_unspent_value` that calculates based on the `fee_rate` and the `vsize` of the unspent.
Can't include the absolute fee in this because it is paid for the whole transaction and not just one unspent - still valid for transactions spending only one unspent.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621826033)
I see, nice!
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621826033)
I see, nice!
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621826528)
I can pick it up in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621826528)
I can pick it up in a follow-up.
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1621830415)
Actually, I can't reproduce it so far. So a corrupt CI machine is more likely.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1621830415)
Actually, I can't reproduce it so far. So a corrupt CI machine is more likely.
💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621836458)
IMO, this 3 deserves a constant soon because it has started showing up in a comment in the functional test as well now besides being in few other places too in this diff. Otherwise, the mind of the code reader wanders off to figuring out where this 3 comes from.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1621836458)
IMO, this 3 deserves a constant soon because it has started showing up in a comment in the functional test as well now besides being in few other places too in this diff. Otherwise, the mind of the code reader wanders off to figuring out where this 3 comes from.