Bitcoin Core Github
44 subscribers
120K links
Download Telegram
πŸ’¬ andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2381032408)
crACK cb9be6729c4e27bf2d6d703c0d454a22cbcdb6e1

This simplifies the CCoinsCacheEntry interface, which was leaking the flags bitfield as an implementation detail. Now we don't have to worry about fuzzing that, and it will make removing the FRESH-but-not-DIRTY or FRESH-and-spent cases easier in https://github.com/bitcoin/bitcoin/pull/30673.

Also, the tests are much saner now and easier to parse by also removing the flags interface there.

There are some minor issues with the commits though.
...
πŸ‘‹ russeree's pull request is ready for review: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
πŸ’¬ russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2381264024)
> 🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30813837174


```
[07:54:50.521] + docker run --rm docker.io/amd64/ubuntu:24.04 bash -c 'env | grep --extended-regexp '\''^(HOME|PATH|USER)='\'''
[07:54:50.522] + tee --append /tmp/env-ci_worker_1726594866_672026932-ci_i686_multiprocess
[07:54:50.522] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
[07:54:50.548] time="2024-09-29T07:54:50Z" level=warning msg="RunRoot is
...
⚠️ russeree opened an issue: "CI : Permission Denied - Multiple PRs"
(https://github.com/bitcoin/bitcoin/issues/30998)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Currently Multiple PRs seem to be failing CI due to permission issues.

https://github.com/bitcoin/bitcoin/pull/30997/checks?check_run_id=30815220500
https://github.com/bitcoin/bitcoin/pull/27260/checks?check_run_id=30814537553

All failures seen happen at approx 12 seconds.

![image](https://github.com/user-attachments/assets/3855d21b-33aa-47f5-88e8-516adfd2f745)


### Expected b
...
πŸ’¬ russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1779998302)
The choice to use chainparams to store the Base58 address prefixes has been utilized in the most recent pull. This solution simplifies the PR a noticeable amount. Long term it would be easier to modify default chain address constants than it would be to maintain the deterministic decoding mechanisms.
πŸ’¬ russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1779998510)
Thank you for this insight, the faulty logic has been removed as a part of the rebase/refactor in the most recent commit.
πŸ“ hebasto opened a pull request: "qt6: Handle deprecated `QLocale::nativeCountryName`"
(https://github.com/bitcoin-core/gui/pull/838)
Split from https://github.com/bitcoin/bitcoin/pull/30997.

[`QLocale::nativeCountryName()`](https://doc.qt.io/qt-6/qlocale-obsolete.html#nativeCountryName) has been deprecated since Qt 6.6.

[`QLocale::nativeTerritoryName()`](https://doc.qt.io/qt-6/qlocale.html#nativeTerritoryName) was introduced in Qt 6.2.

This PR ensures compatibility across all supported Qt versions.

No behaviour change for the current codebase, which uses Qt 5.

---

This PR can be tested on macOS using the fir
...
πŸ“ hebasto opened a pull request: "qt6, test: Handle deprecated code"
(https://github.com/bitcoin-core/gui/pull/839)
Split from https://github.com/bitcoin/bitcoin/pull/30997.

This PR ensures compatibility across all supported Qt versions.

---

This PR can be tested on macOS using the first commit from https://github.com/bitcoin/bitcoin/pull/30997 and Homebrew's `qt` package.
πŸ’¬ tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2381360747)
> Concept ACK
>
> Feels like _p2p_orphan_handling.py_/`test_same_txid_orphan()` might benefit from some augmented checking using this new `getorphantxs`. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.

Great idea. Agreed. A branch was started (https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2327703573) to capture changes to `p2p_orphan_handling.py`. Update
...
πŸ’¬ Khalilheyrani6367 commented on pull request "[refactor] Cleanup BlockAssembler mempool usage":
(https://github.com/bitcoin/bitcoin/pull/28843#issuecomment-2381363217)
60a0f2ac88c57fdf4482dade2ce409ef2da65998
πŸ€” glozow reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2323753194)
Thanks for the review @l0rinc. I've gone through them and addressed some of your comments, and marked some as out of scope. As you can see this PR is fairly large, so it is my preference not to add changes to the code that is being moved. Perhaps we can take some into a followup.

> I'll provide a higher-level review later, once I understand the problem better. I still need a few iterations to be able to zoom out and comment meaningfully, for now I only left nits about implementation specifics
...
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780058381)
done
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772385648)
- Are the other parameters not self-explanatory?
- What do you mean by weird?
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772396787)
Considering this out of scope as it is being moved, not introduced.
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780058788)
We don't know the number of transaction invs ahead of time, so this could reserve a bunch of extra space that is never used. Unsure the best way to handle this - if it's transactions, the size should generally only be a handful anyway.
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780059287)
I think you're referring to the extra space? That's done to leave room for other params to be added in the future, it's more readable.
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780063182)
(Was my first instinct too). That requires we pass `txdownload_impl` to the function, and the fuzz target above doesn't have access to that.
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1780060465)
Agree, changed
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772399108)
I prefer not to assert. My philosophy is that the interface should be simple/robust, i.e. not have a ton of assumptions that would cause the class to fall over if broken. And yes, it means we can easily fuzz that this is the case.
πŸ’¬ glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1772401383)
ms
πŸ’¬ ariard commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2381410581)
I agree with other reviewers so far that we’re better to analyse a set of replacement rules in a logical whole. Be it a holistic upgrade as what could be achieved in the future e.g cluster mempool or replace-by-feerate with partial overlap in the effects on guaranteeing high-fee block templates lingering in the network mempools. On that regard, see the β€œ[On mempool policy consistency](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021116.html)” email thread from few years a
...