📝 jordancampa23-ctrl opened a pull request: "Create Jordan Finesse👌"
(https://github.com/bitcoin/bitcoin/pull/33092)
<!--
*** Please remove the following help
[free.pdf](https://github.com/user-attachments/files/21497432/free.pdf)
[free.pdf](https://github.com/user-attachments/files/21497438/free.pdf)
text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain ho
...
(https://github.com/bitcoin/bitcoin/pull/33092)
<!--
*** Please remove the following help
[free.pdf](https://github.com/user-attachments/files/21497432/free.pdf)
[free.pdf](https://github.com/user-attachments/files/21497438/free.pdf)
text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain ho
...
💬 luke-jr commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2241039496)
Missing a space
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2241039496)
Missing a space
💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2241107374)
fixed. Thanks!
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2241107374)
fixed. Thanks!
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3134232262)
> > should probably lower all three settings, not just the one. However, this breaks a dozen tests or so, there seem to be some more missing that it should be breaking, and feerate estimation currently doesn’t do below 1 s/vB
>
> @murchandamus I'm busy with this to make some progress while @RobinLinus is away, what's the process - create a new PR as a continuation of this one?
Given that this branch is on Robin’s repository, it would be the easiest to create your own branch and PR (unless
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3134232262)
> > should probably lower all three settings, not just the one. However, this breaks a dozen tests or so, there seem to be some more missing that it should be breaking, and feerate estimation currently doesn’t do below 1 s/vB
>
> @murchandamus I'm busy with this to make some progress while @RobinLinus is away, what's the process - create a new PR as a continuation of this one?
Given that this branch is on Robin’s repository, it would be the easiest to create your own branch and PR (unless
...
🤔 mzumsande reviewed a pull request: "init: [gui] Avoid UB/crash in InitAndLoadChainstate"
(https://github.com/bitcoin/bitcoin/pull/32987#pullrequestreview-3069373388)
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
Unrelated, but just mentioning that reindex with the GUI is not great: I tried it with a regtest chain of 1000 empty blocks and the app froze for a few seconds / my OS asked me whether it should terminate the non-responding program. Didn't try it with mainnet, but it's probably not a smooth experience.
(https://github.com/bitcoin/bitcoin/pull/32987#pullrequestreview-3069373388)
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
Unrelated, but just mentioning that reindex with the GUI is not great: I tried it with a regtest chain of 1000 empty blocks and the app froze for a few seconds / my OS asked me whether it should terminate the non-responding program. Didn't try it with mainnet, but it's probably not a smooth experience.
📝 l0rinc opened a pull request: "refactor: remove unused `ser_writedata16be` and `ser_readdata16be`"
(https://github.com/bitcoin/bitcoin/pull/33093)
Remove dead code from serialization logic - extracted from https://github.com/bitcoin/bitcoin/pull/31868
(https://github.com/bitcoin/bitcoin/pull/33093)
Remove dead code from serialization logic - extracted from https://github.com/bitcoin/bitcoin/pull/31868
💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3134384578)
Rebased to solve conflict, the PR is still in draft mode until I remeasure its effect on IBD - and I need to find a nicer way to do the single-byte specializations, since they have a measurable effect but make the code super-ugly... Suggestions are welcome.
In the meantime I have split out the first refactor into https://github.com/bitcoin/bitcoin/pull/33093.
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3134384578)
Rebased to solve conflict, the PR is still in draft mode until I remeasure its effect on IBD - and I need to find a nicer way to do the single-byte specializations, since they have a measurable effect but make the code super-ugly... Suggestions are welcome.
In the meantime I have split out the first refactor into https://github.com/bitcoin/bitcoin/pull/33093.
📝 l0rinc opened a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094)
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To
...
(https://github.com/bitcoin/bitcoin/pull/33094)
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To
...
💬 Christewart commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134394012)
I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?
> This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block.
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134394012)
I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?
> This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block.
💬 kevkevinpal commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3134529754)
ACK [c157438](https://github.com/bitcoin/bitcoin/pull/33083/commits/c1574381168573c561ebddf1945d2debefb340f7)
I was able to reproduce the errors with the diffs provided above, and was also able to validate that the master branch tests pass when the diffs are applied as well
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3134529754)
ACK [c157438](https://github.com/bitcoin/bitcoin/pull/33083/commits/c1574381168573c561ebddf1945d2debefb340f7)
I was able to reproduce the errors with the diffs provided above, and was also able to validate that the master branch tests pass when the diffs are applied as well
💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134531569)
> I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?
Yes it would be good to add a release note saying that `dumptxoutset` can no longer be called with just a filename to produce a snapshot with the latest UTXO state. It now needs an additional `type` argument that can be set to "latest" to match previous behavior. It can also take other arguments and is now capable of rolling back the chain and produci
...
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134531569)
> I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?
Yes it would be good to add a release note saying that `dumptxoutset` can no longer be called with just a filename to produce a snapshot with the latest UTXO state. It now needs an additional `type` argument that can be set to "latest" to match previous behavior. It can also take other arguments and is now capable of rolling back the chain and produci
...
💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134929957)
29.0 is already tagged (archived), built, and released for a long time, so it isn't possible to change the release notes there now.
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134929957)
29.0 is already tagged (archived), built, and released for a long time, so it isn't possible to change the release notes there now.
👋 l0rinc's pull request is ready for review: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094)
(https://github.com/bitcoin/bitcoin/pull/33094)
💬 rkrux commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3135054321)
Thanks for addressing the comments, I will take one final look and then a-c-k.
Tagging @furszy for review as he noticed this issue as well few months back.
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3135054321)
Thanks for addressing the comments, I will take one final look and then a-c-k.
Tagging @furszy for review as he noticed this issue as well few months back.
📝 maflcko opened a pull request: "ci: Remove unused ci/lint_run_all.sh"
(https://github.com/bitcoin/bitcoin/pull/33095)
As per the file content, this can be deleted now. (Stale re-runs are no longer performed for `lint` right now and the CI will be reworked anyway)
(https://github.com/bitcoin/bitcoin/pull/33095)
As per the file content, this can be deleted now. (Stale re-runs are no longer performed for `lint` right now and the CI will be reworked anyway)
✅ maflcko closed a pull request: "ci: Remove unused ci/lint_run_all.sh"
(https://github.com/bitcoin/bitcoin/pull/33095)
(https://github.com/bitcoin/bitcoin/pull/33095)
💬 maflcko commented on pull request "ci: Remove unused ci/lint_run_all.sh":
(https://github.com/bitcoin/bitcoin/pull/33095#issuecomment-3135184921)
Hmm, let's just do it in the other pull.
(https://github.com/bitcoin/bitcoin/pull/33095#issuecomment-3135184921)
Hmm, let's just do it in the other pull.
👍 maflcko approved a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3070236084)
everything looks good and this is probably ready for final review before merge? I just found some nits, which can be done along with follow-ups like https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948. Also, the merge_base issue mentioned above can be addressed in a follow-up.
However, I am not familiar with the GitHub-style CI, including the (docker) caching, but the pull request motivation claims that many are familiar with this, so it should be easy to find reviewers for i
...
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3070236084)
everything looks good and this is probably ready for final review before merge? I just found some nits, which can be done along with follow-ups like https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948. Also, the merge_base issue mentioned above can be addressed in a follow-up.
However, I am not familiar with the GitHub-style CI, including the (docker) caching, but the pull request motivation claims that many are familiar with this, so it should be easy to find reviewers for i
...
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241797039)
nit in 01746e60eb82b9bedb3afa1e791b5abce75ebf75: Unused? This task is using nproc?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241797039)
nit in 01746e60eb82b9bedb3afa1e791b5abce75ebf75: Unused? This task is using nproc?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241816383)
nit in 4a1c5b04691266f1441fd966a4be65140f9a5cc4: Outdated doc?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241816383)
nit in 4a1c5b04691266f1441fd966a4be65140f9a5cc4: Outdated doc?