🤔 furszy reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2034299860)
I'm not completely sure about 584e524eb57444d7192df1049cafde9ccc480406. The commit description says
> Originally these tests verified that at a SelectCoins level that a
> solution with fewer inputs gets preferred at high feerates, and a
> solution with more inputs gets preferred at low feerates. This outcome
> relies on the behavior of BnB, so we move these tests under the umbrella
> of BnB tests.
It is true that the outcome relies only on the BnB behavior currently but that might not
...
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2034299860)
I'm not completely sure about 584e524eb57444d7192df1049cafde9ccc480406. The commit description says
> Originally these tests verified that at a SelectCoins level that a
> solution with fewer inputs gets preferred at high feerates, and a
> solution with more inputs gets preferred at low feerates. This outcome
> relies on the behavior of BnB, so we move these tests under the umbrella
> of BnB tests.
It is true that the outcome relies only on the BnB behavior currently but that might not
...
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1589243941)
In e286c6470b:
No need to clear `expected_result`. It's created within this function.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1589243941)
In e286c6470b:
No need to clear `expected_result`. It's created within this function.
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586712079)
In 3dff8f0490:
As far as I can see, the `nInput` arg is always 0 in this PR and also in #28985. Can we remove it?
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586712079)
In 3dff8f0490:
As far as I can see, the `nInput` arg is always 0 in this PR and also in #28985. Can we remove it?
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586720038)
In https://github.com/bitcoin/bitcoin/commit/3dff8f0490b4b92c4adf229a828063dfda6d3a80:
I don't think we need all this includes. For instance, no wallet is used and `<wallet/wallet.h>` is included, no fee function is used and `wallet/fees.h` is included, `<random.h>` and `<random>` are included.
Maybe run IOWY to clean this up.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586720038)
In https://github.com/bitcoin/bitcoin/commit/3dff8f0490b4b92c4adf229a828063dfda6d3a80:
I don't think we need all this includes. For instance, no wallet is used and `<wallet/wallet.h>` is included, no fee function is used and `wallet/fees.h` is included, `<random.h>` and `<random>` are included.
Maybe run IOWY to clean this up.
💬 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_r1589340861)
In that case, maybe have a different and unconditional error message / keep the assert just for the case where pruning isn't enabled?
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589340861)
In that case, maybe have a different and unconditional error message / keep the assert just for the case where pruning isn't enabled?
💬 willcl-ark commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093222242)
> I would slightly prefer links in the two files I wrote (libraries.md and multiprocess.md) to be relative so they work with emacs and so links within the files use a consistent style
A rough search of *.md files shows me that we have 87 absolute links and 72 relative:
<details>
<summary>Details</summary>
```fish
will@ubuntu in ~/src/bitcoin on fix-links [$!?] : 🐍 (bitcoin)
₿ rg "\]\(\/" */**.md | wc -l
87
will@ubuntu in ~/src/bitcoin on fix-links [$!?] : 🐍 (bitcoin)
₿ rg
...
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093222242)
> I would slightly prefer links in the two files I wrote (libraries.md and multiprocess.md) to be relative so they work with emacs and so links within the files use a consistent style
A rough search of *.md files shows me that we have 87 absolute links and 72 relative:
<details>
<summary>Details</summary>
```fish
will@ubuntu in ~/src/bitcoin on fix-links [$!?] : 🐍 (bitcoin)
₿ rg "\]\(\/" */**.md | wc -l
87
will@ubuntu in ~/src/bitcoin on fix-links [$!?] : 🐍 (bitcoin)
₿ rg
...
👍 willcl-ark approved a pull request: "test: remove duplicate `WITNESS_SCALE_FACTOR` constant defination"
(https://github.com/bitcoin/bitcoin/pull/30029#pullrequestreview-2038395689)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
(https://github.com/bitcoin/bitcoin/pull/30029#pullrequestreview-2038395689)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
💬 andrewtoth commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589357333)
Indeed, I think that's the best way. I did that before https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1375494479 but other reviewers thought removing the assert is better. But, I think it makes it easier to merge this PR if there is strictly no behavior change, and other patches can be proposed to modify the assertion behavior or disconnecting instead of returning early.
So, if reading the block fails, we retake `cs_main` and determine if the block was pruned out from under us. If
...
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1589357333)
Indeed, I think that's the best way. I did that before https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1375494479 but other reviewers thought removing the assert is better. But, I think it makes it easier to merge this PR if there is strictly no behavior change, and other patches can be proposed to modify the assertion behavior or disconnecting instead of returning early.
So, if reading the block fails, we retake `cs_main` and determine if the block was pruned out from under us. If
...
💬 sipa commented on pull request "test: remove duplicate `WITNESS_SCALE_FACTOR` constant defination":
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093249622)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
(https://github.com/bitcoin/bitcoin/pull/30029#issuecomment-2093249622)
ACK af3c18169a075222fe0795ab24b8b20ad5e30ae4
💬 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
...