💬 mzumsande commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589367166)
> determine if the block was pruned out from under us
That sounds like work, not sure how easy it is - I just meant to check whether we're running in pruning mode, so the block could have been pruned theoretically. I don't have a strong opinion between asserting / logging an error unconditionally, I just think that a conditional NET log is not sufficient in a case where something is seriously wrong on our end.
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589367166)
> determine if the block was pruned out from under us
That sounds like work, not sure how easy it is - I just meant to check whether we're running in pruning mode, so the block could have been pruned theoretically. I don't have a strong opinion between asserting / logging an error unconditionally, I just think that a conditional NET log is not sufficient in a case where something is seriously wrong on our end.
👍 pinheadmz approved a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2038429816)
ACK a0ebb929e865903ca1cc2674e74906a806e73109
Diff since last ACK is minimal, just nits in tests. Since it's been a while, I re-reviewed the whole PR anyway. Most of the changes are cleanup refactors in the relevant tests which I agree are worthwhile. The main bugfix is switching from `FillableSigningProvider` to `FlatSigningProvider` when adding a CScript in`AddAndGetDestinationForScript()`, because the latter does not require that `redeemScript.size() <= MAX_SCRIPT_ELEMENT_SIZE`
The scrip
...
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2038429816)
ACK a0ebb929e865903ca1cc2674e74906a806e73109
Diff since last ACK is minimal, just nits in tests. Since it's been a while, I re-reviewed the whole PR anyway. Most of the changes are cleanup refactors in the relevant tests which I agree are worthwhile. The main bugfix is switching from `FillableSigningProvider` to `FlatSigningProvider` when adding a CScript in`AddAndGetDestinationForScript()`, because the latter does not require that `redeemScript.size() <= MAX_SCRIPT_ELEMENT_SIZE`
The scrip
...
💬 sipa commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093264723)
Incorporated the changes from #30031.
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093264723)
Incorporated the changes from #30031.
✅ hebasto closed a pull request: "msvc: Compile `test\fuzz\miniscript.cpp`"
(https://github.com/bitcoin/bitcoin/pull/30031)
(https://github.com/bitcoin/bitcoin/pull/30031)
💬 hebasto commented on pull request "msvc: Compile `test\fuzz\miniscript.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30031#issuecomment-2093267887)
Closing in favour of #28657.
(https://github.com/bitcoin/bitcoin/pull/30031#issuecomment-2093267887)
Closing in favour of #28657.
👍 hebasto approved a pull request: "miniscript: make operator""_mst consteval"
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038441451)
re-ACK 63317103c9f2b0635558da814567bb79c17ae851.
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038441451)
re-ACK 63317103c9f2b0635558da814567bb79c17ae851.
📝 hebasto reopened a pull request: "msvc: Compile `test\fuzz\miniscript.cpp`"
(https://github.com/bitcoin/bitcoin/pull/30031)
Based on https://github.com/bitcoin/bitcoin/pull/28657.
From the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/8939189856/job/24554760656?pr=30031#step:29:233):
```
miniscript_script: succeeded against 721 files in 1s.
Run miniscript_script with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_script')]
miniscript_smart: succeeded against 1429 files in 2s.
Run miniscript_smart with args ['D:\\a\\bitcoin\\bitcoin\\src
...
(https://github.com/bitcoin/bitcoin/pull/30031)
Based on https://github.com/bitcoin/bitcoin/pull/28657.
From the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/8939189856/job/24554760656?pr=30031#step:29:233):
```
miniscript_script: succeeded against 721 files in 1s.
Run miniscript_script with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_script')]
miniscript_smart: succeeded against 1429 files in 2s.
Run miniscript_smart with args ['D:\\a\\bitcoin\\bitcoin\\src
...
👍 theuni approved a pull request: "miniscript: make operator""_mst consteval"
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038454092)
utACK 63317103c9f2b0635558da814567bb79c17ae851
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038454092)
utACK 63317103c9f2b0635558da814567bb79c17ae851
📝 instagibbs opened a pull request: "p2p: Allow 1P1C to fetch parent from compact block extra_txn"
(https://github.com/bitcoin/bitcoin/pull/30032)
One relatively common pattern of 1P1C relay is receiving
a just-below-minfee parent, dropping it and marking it
as reconsiderable, then fetching it again from the peer
once the child is added to the orphanage.
A cache of dropped parents would be useful, and it turns
out we're already opportunistically storing transactions
like this for compact blocks as "extra transactions".
Use this size-limited cache to potentially fetch a
reconsiderable parent, and submit for validation.
(https://github.com/bitcoin/bitcoin/pull/30032)
One relatively common pattern of 1P1C relay is receiving
a just-below-minfee parent, dropping it and marking it
as reconsiderable, then fetching it again from the peer
once the child is added to the orphanage.
A cache of dropped parents would be useful, and it turns
out we're already opportunistically storing transactions
like this for compact blocks as "extra transactions".
Use this size-limited cache to potentially fetch a
reconsiderable parent, and submit for validation.
💬 vostrnad commented on pull request "test: remove duplicate `WITNESS_SCALE_FACTOR` constant defination":
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093314146)
nit: typo in PR name (defination -> definition)
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093314146)
nit: typo in PR name (defination -> definition)
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093325471)
> @fjahr's earlier code (https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2035943330) did not change consensus.powLimit but instead changed GetNextWorkRequired to enforce this. As a consequence the initial blocks after genesis could have difficulty 1, but once the difficulty goes above a typical S9, it can no longer drop below that.
I did implement an exception to allow the difficulty to drop below the adjustment (1M in this case), see my comment on this in the old commit: http
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093325471)
> @fjahr's earlier code (https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2035943330) did not change consensus.powLimit but instead changed GetNextWorkRequired to enforce this. As a consequence the initial blocks after genesis could have difficulty 1, but once the difficulty goes above a typical S9, it can no longer drop below that.
I did implement an exception to allow the difficulty to drop below the adjustment (1M in this case), see my comment on this in the old commit: http
...
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1589417605)
Added, sorry I forgot about that.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1589417605)
Added, sorry I forgot about that.
💬 achow101 commented on pull request "test: remove duplicate `WITNESS_SCALE_FACTOR` constant definition":
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093367695)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093367695)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
🚀 achow101 merged a pull request: "test: remove duplicate `WITNESS_SCALE_FACTOR` constant definition"
(https://github.com/bitcoin/bitcoin/pull/30029)
(https://github.com/bitcoin/bitcoin/pull/30029)
💬 achow101 commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093369038)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093369038)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
🚀 achow101 merged a pull request: "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE"
(https://github.com/bitcoin/bitcoin/pull/30024)
(https://github.com/bitcoin/bitcoin/pull/30024)
💬 furszy commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589443852)
> I just meant to check whether we're running in pruning mode, so the block could have been pruned theoretically.
Just in case, it should also check that the block is further than the pruning threshold.
> So, if reading the block fails, we retake cs_main and determine if the block was pruned out from under us. If so, we log and return early. Otherwise, we assert as before.
For now (at least in this PR), we should maintain the current network behavior if the assert is replaced. This mean
...
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589443852)
> I just meant to check whether we're running in pruning mode, so the block could have been pruned theoretically.
Just in case, it should also check that the block is further than the pruning threshold.
> So, if reading the block fails, we retake cs_main and determine if the block was pruned out from under us. If so, we log and return early. Otherwise, we assert as before.
For now (at least in this PR), we should maintain the current network behavior if the assert is replaced. This mean
...
🤔 furszy reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2038687143)
sad rebase after #30024 merge.
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2038687143)
sad rebase after #30024 merge.
💬 fanquake commented on pull request "depends: swap some cctools tools for LLVM tools":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2093465723)
Guix Build (aarch64)
```bash
6a215012d268b68b25d3120c63f7154169061d71bef8e3a434dc44d6e2c8bf06 guix-build-4313febae66b/output/aarch64-linux-gnu/SHA256SUMS.part
7469b754ce7632c175116ffb1490771b9a4b635c33ccf711fd5df2cf08c6d6d4 guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu-debug.tar.gz
7ffd30c4e1894555a1172e13106d8fbf6291f357073fb17840e653f98fe3599f guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu.tar.gz
f8d0a35
...
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2093465723)
Guix Build (aarch64)
```bash
6a215012d268b68b25d3120c63f7154169061d71bef8e3a434dc44d6e2c8bf06 guix-build-4313febae66b/output/aarch64-linux-gnu/SHA256SUMS.part
7469b754ce7632c175116ffb1490771b9a4b635c33ccf711fd5df2cf08c6d6d4 guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu-debug.tar.gz
7ffd30c4e1894555a1172e13106d8fbf6291f357073fb17840e653f98fe3599f guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu.tar.gz
f8d0a35
...
💬 pinheadmz commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#issuecomment-2093475353)
I wrote a test for RPC `signrawtransactionwithkey` that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds `MAX_STANDARD_SCRIPTSIG_SIZE` and if it ever made it into a block, would exceed `MAX_SCRIPT_ELEMENT_SIZE`
This test behaves the same on master! So, that is potentially a footgun although not an easy one for a user to pull off and also would not result in any loss of funds.
https://gist.github.com/pinheadmz/ca8ed75
...
(https://github.com/bitcoin/bitcoin/pull/28307#issuecomment-2093475353)
I wrote a test for RPC `signrawtransactionwithkey` that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds `MAX_STANDARD_SCRIPTSIG_SIZE` and if it ever made it into a block, would exceed `MAX_SCRIPT_ELEMENT_SIZE`
This test behaves the same on master! So, that is potentially a footgun although not an easy one for a user to pull off and also would not result in any loss of funds.
https://gist.github.com/pinheadmz/ca8ed75
...