💬 maflcko commented on issue "ci: Support running from a worktree":
(https://github.com/bitcoin/bitcoin/issues/30028#issuecomment-2093142760)
I guess `macos` can just be skipped, as it will never run the base-install twice, so doesn't need caching.
(https://github.com/bitcoin/bitcoin/issues/30028#issuecomment-2093142760)
I guess `macos` can just be skipped, as it will never run the base-install twice, so doesn't need caching.
💬 jonatack commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093181592)
Thanks for the interesting links!
@GregTonoski it turns out that this patch is just a continuation of commit 192cc910ec7cade1d0dce7f3b111e7fc7720e607 doing the same thing, per review https://github.com/bitcoin/bitcoin/pull/2188#discussion_r2703506 requesting this, both back in 2013.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093181592)
Thanks for the interesting links!
@GregTonoski it turns out that this patch is just a continuation of commit 192cc910ec7cade1d0dce7f3b111e7fc7720e607 doing the same thing, per review https://github.com/bitcoin/bitcoin/pull/2188#discussion_r2703506 requesting this, both back in 2013.
💬 naiyoma commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1589320996)
@tdb3 I've pushed an [update](https://github.com/bitcoin/bitcoin/pull/29858/commits/e02af85abaec67d92e976383777a4b0f2caae4e1). Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.
I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn't. Below is a sample output
...
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1589320996)
@tdb3 I've pushed an [update](https://github.com/bitcoin/bitcoin/pull/29858/commits/e02af85abaec67d92e976383777a4b0f2caae4e1). Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.
I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn't. Below is a sample output
...
💬 1440000bytes commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2093184542)
Concept ACK on adding another DNS seeder
> > Why does the seeder consider 'default port' for good nodes?
>
> DNS cannot provide port numbers, but a port must be known when connecting to a node. So we assume the default port, and because of that assumption, DNS seeders need to return nodes that are listening on the default port.
TXT records could work but that will require lot of other changes (out of scope)
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2093184542)
Concept ACK on adding another DNS seeder
> > Why does the seeder consider 'default port' for good nodes?
>
> DNS cannot provide port numbers, but a port must be known when connecting to a node. So we assume the default port, and because of that assumption, DNS seeders need to return nodes that are listening on the default port.
TXT records could work but that will require lot of other changes (out of scope)
🤔 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
...