💬 optout21 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3475810317)
Concept ACK 61cd0e53ca45684cd5ef9084829ecc6d59154595
I notice that `CTransaction::ToString()` prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Options:
- keep the duplication
- remove duplication, change the order
- remove the duplication and keep the order -- there should be a version of `CTxIn::ToString()` withou
...
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3475810317)
Concept ACK 61cd0e53ca45684cd5ef9084829ecc6d59154595
I notice that `CTransaction::ToString()` prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Options:
- keep the duplication
- remove duplication, change the order
- remove the duplication and keep the order -- there should be a version of `CTxIn::ToString()` withou
...
💬 Eunovo commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2483136940)
> It should pass because we still return (The issue with commenting that line is that subsequent wait for same block template will return instantly). We can add a test case to verify that after interrupting a wait, you can wait again.
Yes. I commented out the line on purpose to discover ways to improve the tests.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2483136940)
> It should pass because we still return (The issue with commenting that line is that subsequent wait for same block template will return instantly). We can add a test case to verify that after interrupting a wait, you can wait again.
Yes. I commented out the line on purpose to discover ways to improve the tests.
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3475819828)
I suggest splitting into two commits:
- Preparatory rename & new parameter(s) but without behavior change, to minimize diff in 2nd commit (rename ALWAYS to FORCE_FLUSH I suppose)
- Change in behavior in the respective places (`scantxoutset`, `gettxoutsetinfo`, etc.)
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3475819828)
I suggest splitting into two commits:
- Preparatory rename & new parameter(s) but without behavior change, to minimize diff in 2nd commit (rename ALWAYS to FORCE_FLUSH I suppose)
- Change in behavior in the respective places (`scantxoutset`, `gettxoutsetinfo`, etc.)
💬 optout21 commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2483143005)
Maybe this could come before the derivation-path-filling loop above. That way the early exit condition on missing sighash comes earlier, and the loop does not have to be broken into two.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2483143005)
Maybe this could come before the derivation-path-filling loop above. That way the early exit condition on missing sighash comes earlier, and the loop does not have to be broken into two.
💬 optout21 commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2483144708)
It seems the proper, but much higher impact solution would be to change the `nHashType` field to `uint8_t`, what do you think? Maybe create a separate issue or PR for that?
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2483144708)
It seems the proper, but much higher impact solution would be to change the `nHashType` field to `uint8_t`, what do you think? Maybe create a separate issue or PR for that?
💬 optout21 commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3475833560)
Concept ACK 000c685bcdd395d61a58ad9698563cab41e2d7d0
(more review needed)
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3475833560)
Concept ACK 000c685bcdd395d61a58ad9698563cab41e2d7d0
(more review needed)
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3475945645)
Rebased the latest version of the PR now that https://github.com/bitcoin/bitcoin/pull/31645 was merged and measured a reindex on my M4 laptop: it finished all 3 runs with dbcache 450, 4500 and 45000 overnight.
<details>
<summary>Details</summary>
time ./build/bin/bitcoind -stopatheight=921129 -dbcache=45000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 \
&& time ./build/bin/bitcoind -stopatheight=921129 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconso
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3475945645)
Rebased the latest version of the PR now that https://github.com/bitcoin/bitcoin/pull/31645 was merged and measured a reindex on my M4 laptop: it finished all 3 runs with dbcache 450, 4500 and 45000 overnight.
<details>
<summary>Details</summary>
time ./build/bin/bitcoind -stopatheight=921129 -dbcache=45000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 \
&& time ./build/bin/bitcoind -stopatheight=921129 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconso
...
⚠️ Mdhlikoo-create opened an issue: "Bitcoin free 🆓"
(https://github.com/bitcoin/bitcoin/issues/33760)
### Please describe the feature you'd like to see added.
Bitcoin you can use as a community
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
(https://github.com/bitcoin/bitcoin/issues/33760)
### Please describe the feature you'd like to see added.
Bitcoin you can use as a community
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
✅ willcl-ark closed an issue: "Bitcoin free 🆓"
(https://github.com/bitcoin/bitcoin/issues/33760)
(https://github.com/bitcoin/bitcoin/issues/33760)
🤔 TheCharlatan reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407450551)
Thanks for the review @stringintech!
Updated 1ea43dc365c277f519f30f55dae6b0899e611765 -> a060f3296b6191dde03e4dd8d956ffdf19abca0b ([kernelApi_78](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_78) -> [kernelApi_79](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_79), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_78..kernelApi_79))
* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474390216), cleaned up frie
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407450551)
Thanks for the review @stringintech!
Updated 1ea43dc365c277f519f30f55dae6b0899e611765 -> a060f3296b6191dde03e4dd8d956ffdf19abca0b ([kernelApi_78](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_78) -> [kernelApi_79](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_79), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_78..kernelApi_79))
* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474390216), cleaned up frie
...
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483641160)
I don't think this is strictly necessary, since we only ever have a single validation interface registered internally at a time. However, I think we should mimic our own internal code's behavior here and always unregister manually first.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483641160)
I don't think this is strictly necessary, since we only ever have a single validation interface registered internally at a time. However, I think we should mimic our own internal code's behavior here and always unregister manually first.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483649981)
I ended up removing this altogether. This was introduced before I had a coherent flushing strategy for the chainman and before we had smarter regular flushing in validation. As such, I don't think it is useful anymore to force a flush here. Maybe we can re-introduce it when we eventually expose interfaces for allowing programmers to control the way data is written.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483649981)
I ended up removing this altogether. This was introduced before I had a coherent flushing strategy for the chainman and before we had smarter regular flushing in validation. As such, I don't think it is useful anymore to force a flush here. Maybe we can re-introduce it when we eventually expose interfaces for allowing programmers to control the way data is written.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483657499)
Yes, we might expose more information here later, and the split between between the enums might also be useful if we ever expose a `TxValidationState`.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483657499)
Yes, we might expose more information here later, and the split between between the enums might also be useful if we ever expose a `TxValidationState`.
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3476337221)
Bitcoin Core relay policy is largely motivated by not relaying third party malleable transactions. There is no reason to relay transactions if they are just going to be modified before they get mined.
Given the, unfortunately extensive, amount of malleability generally afford by Bitcoin Script's design, sometimes policy just limits third party malleable transactions to some canonical form, one that minimizes transaction size so that miners are not incentivized to reduce the size further. He
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3476337221)
Bitcoin Core relay policy is largely motivated by not relaying third party malleable transactions. There is no reason to relay transactions if they are just going to be modified before they get mined.
Given the, unfortunately extensive, amount of malleability generally afford by Bitcoin Script's design, sometimes policy just limits third party malleable transactions to some canonical form, one that minimizes transaction size so that miners are not incentivized to reduce the size further. He
...
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3476339242)
@TheCharlatan I touch on this in the above, but I do want to explicitly note that the order of public keys that Counterparty uses changed in response to #5247 and the [the keys used to be in the other order](https://github.com/CounterpartyXCP/counterparty-core/commit/f1a8c82914b2f913560496e3d1becc1b63f88118#diff-b9154ac64e110b5561c5bcadc77b532aaea3f44eb53e9c4a76ae2ed3c6fad8b8L231-R261).
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3476339242)
@TheCharlatan I touch on this in the above, but I do want to explicitly note that the order of public keys that Counterparty uses changed in response to #5247 and the [the keys used to be in the other order](https://github.com/CounterpartyXCP/counterparty-core/commit/f1a8c82914b2f913560496e3d1becc1b63f88118#diff-b9154ac64e110b5561c5bcadc77b532aaea3f44eb53e9c4a76ae2ed3c6fad8b8L231-R261).
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3476389342)
Rebased daf0e9a3d45f42889fc5895fc580c73d060d2711 -> 00c5a8736532a0ca3c483d300e1d09d87be948f1 ([blocktreestore_5](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_5) -> [blocktreestore_6](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_5..blocktreestore_6))
* Fixed conflict with #31645
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3476389342)
Rebased daf0e9a3d45f42889fc5895fc580c73d060d2711 -> 00c5a8736532a0ca3c483d300e1d09d87be948f1 ([blocktreestore_5](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_5) -> [blocktreestore_6](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_5..blocktreestore_6))
* Fixed conflict with #31645
💬 oren-z0 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483736256)
Why make verbosity optional, and not keep as-is?
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483736256)
Why make verbosity optional, and not keep as-is?
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483749818)
> Why make verbosity optional, and not keep as-is?
It allows reusing this function also for the new REST endpoint (`/blockpart/`) which doesn't support JSON response format.
> Where is it called with a null value?
It is called by `rest_block_part()` here:
https://github.com/romanz/bitcoin/blob/c30647c4d34c2941696729704854467b30657c43/src/rest.cpp#L502
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483749818)
> Why make verbosity optional, and not keep as-is?
It allows reusing this function also for the new REST endpoint (`/blockpart/`) which doesn't support JSON response format.
> Where is it called with a null value?
It is called by `rest_block_part()` here:
https://github.com/romanz/bitcoin/blob/c30647c4d34c2941696729704854467b30657c43/src/rest.cpp#L502
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483807860)
Can we hoist this check out before the `try_emplace`, and just `continue` if this is the case?
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483807860)
Can we hoist this check out before the `try_emplace`, and just `continue` if this is the case?
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483809503)
I also think it would make sense to have a default `true`. However, if we want to be more explicit about intent to reuse the cache rather than implementation details of what we do if we don't, should we rename this parameter something more descriptive like `will_reuse`? That is the first thing that came to mind, maybe there are better names.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483809503)
I also think it would make sense to have a default `true`. However, if we want to be more explicit about intent to reuse the cache rather than implementation details of what we do if we don't, should we rename this parameter something more descriptive like `will_reuse`? That is the first thing that came to mind, maybe there are better names.