📝 hebasto opened a pull request: "doc: Adjust path in comment"
(https://github.com/bitcoin/bitcoin/pull/32056)
It was overlooked in bitcoin/bitcoin#31161.
(https://github.com/bitcoin/bitcoin/pull/32056)
It was overlooked in bitcoin/bitcoin#31161.
💬 hebasto commented on pull request "doc: Adjust path in comment":
(https://github.com/bitcoin/bitcoin/pull/32056#issuecomment-2720986013)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/32056#issuecomment-2720986013)
cc @dergoegge
👍 dergoegge approved a pull request: "doc: Adjust path in comment"
(https://github.com/bitcoin/bitcoin/pull/32056#pullrequestreview-2681604534)
ACK de1ada079bf52b9d751bbeb1e7bb3011beb62429
(https://github.com/bitcoin/bitcoin/pull/32056#pullrequestreview-2681604534)
ACK de1ada079bf52b9d751bbeb1e7bb3011beb62429
💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2721005512)
I believe it's possible for effective_value to _not_ be equal between two successive UTXO combinations while two fees _are_ equal. This would have the consequence of _not_ applying the optimization when in principle the optimization _should_ be applied. In fact, I think it's very likely for two fees to be equal since it's likely the fee_rates are equal and the weight is equal of two UTXOs. Therefore, leaving this condition in has the disadvantage of nullifying the effect of the UTXO optimizat
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2721005512)
I believe it's possible for effective_value to _not_ be equal between two successive UTXO combinations while two fees _are_ equal. This would have the consequence of _not_ applying the optimization when in principle the optimization _should_ be applied. In fact, I think it's very likely for two fees to be equal since it's likely the fee_rates are equal and the weight is equal of two UTXOs. Therefore, leaving this condition in has the disadvantage of nullifying the effect of the UTXO optimizat
...
💬 moonsettler commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1993374840)
Computation required by CTV ideally would not be carried out when CTV could not have been active.
In this context the `tx.version` field is available. If CTV required version 3 or higher (not sure this is a good idea or not?), then it would be trivial to augment the old logic that was more conservative with resource use. (again not sure how much that matters in practice?)
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1993374840)
Computation required by CTV ideally would not be carried out when CTV could not have been active.
In this context the `tx.version` field is available. If CTV required version 3 or higher (not sure this is a good idea or not?), then it would be trivial to augment the old logic that was more conservative with resource use. (again not sure how much that matters in practice?)
💬 Nouridin commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2721031585)
Your reasoning is sound. If you consider that many UTXOs will have equal fees—because they share the same fee rate and weight—then requiring both the effective value and the fee to match before applying the shortcut can indeed be overly strict. In scenarios where the fees are identical but the effective values differ slightly (perhaps due to minor rounding or variation in the UTXO values), the condition
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee`
will cause the optimization to be bypass
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2721031585)
Your reasoning is sound. If you consider that many UTXOs will have equal fees—because they share the same fee rate and weight—then requiring both the effective value and the fee to match before applying the shortcut can indeed be overly strict. In scenarios where the fees are identical but the effective values differ slightly (perhaps due to minor rounding or variation in the UTXO values), the condition
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee`
will cause the optimization to be bypass
...
💬 l0rinc commented on pull request "doc: Adjust path in comment":
(https://github.com/bitcoin/bitcoin/pull/32056#issuecomment-2721060655)
utACK de1ada079bf52b9d751bbeb1e7bb3011beb62429
Searched the code couldn't find any other obvious leftovers
(https://github.com/bitcoin/bitcoin/pull/32056#issuecomment-2721060655)
utACK de1ada079bf52b9d751bbeb1e7bb3011beb62429
Searched the code couldn't find any other obvious leftovers
🚀 fanquake merged a pull request: "doc: Adjust path in comment"
(https://github.com/bitcoin/bitcoin/pull/32056)
(https://github.com/bitcoin/bitcoin/pull/32056)
💬 yancyribbens commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721071281)
Concept ACK. It looks like the result of the sample changes is less casting which is a good sign.
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721071281)
Concept ACK. It looks like the result of the sample changes is less casting which is a good sign.
🤔 l0rinc reviewed a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2681684064)
post-merge ACK
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2681684064)
post-merge ACK
💬 l0rinc commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1993404965)
post-merge nit: if you touch again please realign the comments
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1993404965)
post-merge nit: if you touch again please realign the comments
📝 Sjors opened a pull request: "test: testnet4 could log a disk space warning"
(https://github.com/bitcoin/bitcoin/pull/32057)
`feature_config_args.py` incorrectly assumed that its testnet4 node would not log a disk space warning for testnet4.
But when #31978 increased `m_assumed_blockchain_size` on testnet4 from 1 to 11 GiB, it triggered this bug on my RAM disk, see https://github.com/bitcoin/bitcoin/tree/master/test#speed-up-test-runs-with-a-ram-disk
This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
(https://github.com/bitcoin/bitcoin/pull/32057)
`feature_config_args.py` incorrectly assumed that its testnet4 node would not log a disk space warning for testnet4.
But when #31978 increased `m_assumed_blockchain_size` on testnet4 from 1 to 11 GiB, it triggered this bug on my RAM disk, see https://github.com/bitcoin/bitcoin/tree/master/test#speed-up-test-runs-with-a-ram-disk
This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
💬 Sjors commented on pull request "test: testnet4 could log a disk space warning":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2721109159)
cc @fjahr
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2721109159)
cc @fjahr
💬 ajtowns commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2721136168)
> Something like this? Conceptually seems useful, but here I don't know how to interpret it, seems too far zoomed in
Fair; that might work better with a time on the x-axis rather than height, something like:
```python
for time_commit in [time_commit_X, time_commit_Y, time_commit_Z]:
for height in range(1,850000):
y = time_baseline[height] / time_commit[height] # keep this one the same
x = time_baseline[height] # changed from x = height
add_datapoint(x,y)
...
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2721136168)
> Something like this? Conceptually seems useful, but here I don't know how to interpret it, seems too far zoomed in
Fair; that might work better with a time on the x-axis rather than height, something like:
```python
for time_commit in [time_commit_X, time_commit_Y, time_commit_Z]:
for height in range(1,850000):
y = time_baseline[height] / time_commit[height] # keep this one the same
x = time_baseline[height] # changed from x = height
add_datapoint(x,y)
...
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993431200)
If accepting a new argument can't be avoided for this PSBT RPC because of the same `CreateTxDoc()` being reused underneath, then I think we can explore updating functional tests for PSBT in this PR itself because `createpsbt` RPC uses the same `ConstructTransaction()` as well.
`rpc_psbt.py` is the file that would need to be updated. Using a similar approach like done in the `rpc_rawtransaction` would be preferred that is reusing bunch of the existing testing methods.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993431200)
If accepting a new argument can't be avoided for this PSBT RPC because of the same `CreateTxDoc()` being reused underneath, then I think we can explore updating functional tests for PSBT in this PR itself because `createpsbt` RPC uses the same `ConstructTransaction()` as well.
`rpc_psbt.py` is the file that would need to be updated. Using a similar approach like done in the `rpc_rawtransaction` would be preferred that is reusing bunch of the existing testing methods.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993416704)
Although this works but there seems to be a note here regarding legacy wallets, which are being deprecated. I think a more suitable place could be in the `getrawtransaction_verbosity_tests` that use `create_self_transfer()`, which already accepts a version argument in the tests - just need to pass down 3.
Also, testing the value of the transaction version after being mined would complete the all-round testing of this functionality for which only adding a new `field` for version in the `getraw
...
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993416704)
Although this works but there seems to be a note here regarding legacy wallets, which are being deprecated. I think a more suitable place could be in the `getrawtransaction_verbosity_tests` that use `create_self_transfer()`, which already accepts a version argument in the tests - just need to pass down 3.
Also, testing the value of the transaction version after being mined would complete the all-round testing of this functionality for which only adding a new `field` for version in the `getraw
...
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993433641)
Is this comment outdated? I don't understand where are the default args here that are referred to in the above comment.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993433641)
Is this comment outdated? I don't understand where are the default args here that are referred to in the above comment.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993326051)
I am inclining towards not adding a positional argument for this. Mostly because this is not something the user would use frequently and instead would let the defaults take over. I think using an optional `options` object here (used in many RPCs) would suffice for this use case.
@glozow WDYT?
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993326051)
I am inclining towards not adding a positional argument for this. Mostly because this is not something the user would use frequently and instead would let the defaults take over. I think using an optional `options` object here (used in many RPCs) would suffice for this use case.
@glozow WDYT?
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993398098)
Let's use `uint32_t` because that is what is used in both `CTransaction` and `CMutableTransaction`.
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L308
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L381
Those docs might be outdated because this change was done recently: https://github.com/bitcoin/bitcoin/pull/29325
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993398098)
Let's use `uint32_t` because that is what is used in both `CTransaction` and `CMutableTransaction`.
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L308
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L381
Those docs might be outdated because this change was done recently: https://github.com/bitcoin/bitcoin/pull/29325
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993472740)
See: https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993416704
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993472740)
See: https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993416704