Bitcoin Core Github
43 subscribers
122K links
Download Telegram
πŸ‘ l0rinc approved a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3484132142)
untested code review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa

This is a continuation of https://github.com/bitcoin/bitcoin/pull/30660/files#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR326, separating the existing `TimeoutError` to add extra details for `ConnectionResetError` as well.
πŸ’¬ fjahr commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3554086620)
> Good catch. The test needs to check two things:

Thanks for explaining, I have switched the check to node0 and kept it above the sync because that feels like the most natural flow given my improved understanding of the test now. I have also added some short comments based on your explanation.
πŸ’¬ D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3554088845)
with [36ae2ef](https://github.com/bitcoin/bitcoin/commit/36ae2effba30f6806a6171fa6a8018fde5215302) rebased over master.

No changes to commits...
πŸ’¬ ryanofsky commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3554122176)
re: https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3553506888

> This push is mainly for a CI sanity check, then I'm going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in `HTTPServer`

I agree with theuni it should make this PR easier to review and simpler if current sockman functionality ([sockman.cpp](https://github.com/pinheadmz/bitcoin/blob/8c81bdf2532aa1c61e25d367a480ce3aa7
...
πŸ’¬ hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3554161151)
> > minimum GCC version for Windows cross-compilation to [13.1](https://doc.qt.io/qt-6.8/supported-platforms.html#windows).
>
> Have you checked that it doesn't work with 12?

This [branch](https://github.com/hebasto/bitcoin/tree/251119-qt6.9) fails to cross-compile on Debian Bookworm with GCC 12.2.0:
```
/home/hebasto/bitcoin/depends/work/build/x86_64-w64-mingw32/qt/6.9.3-d5ebe8f681b/qtbase/src/3rdparty/pcre2/src/pcre2_compile.c: In function β€˜pcre2_compile_16’:
/home/hebasto/bitcoin/dep
...
πŸ’¬ mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2543210150)
Done. Since the `net_processing` warning is now harder to understand (half of the logged text comes from validation) I added a comment there.
πŸ’¬ mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3554211685)
[6db2551](https://github.com/bitcoin/bitcoin/commit/6db2551dc27c4a9b989e8814054c93dd9d8f1b36) to [7f28483](https://github.com/bitcoin/bitcoin/commit/7f284835be87c4d1a8a56804992043e12dae1ea1):
- addressed https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2539190197
- reworked the second `CheckForkWarningConditions` commit to unify the log message with the alernotify warning. I noticed the discrepancy due to #33893
πŸ€” mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3484323098)
Concept ACK

FYI I proposed to reword the warning message in #33553 (7f284835be87c4d1a8a56804992043e12dae1ea1).
πŸ’¬ Sjors commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3554275116)
utACK 944502628a532469b0d3c9a3af04fa2d9c18f849
πŸ’¬ mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2543275564)
@polespinasa Not sure (#15141 seems relevant), but I think it's to avoid that the network splits into two parts in case of bugs leading to an temporary consensus incompability between clients, that may self-correct because the invalid block may be reorged out.
πŸ’¬ maflcko commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#discussion_r2543309283)

it own -> its own [possessive pronoun "its" required; "it" is incorrect here]

drahtbot_id_5_m
πŸ’¬ maflcko commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3554424316)
I'd say it is fine to bump the minimum required to 13 for win-cross compilation:

```diff
diff --git a/doc/build-windows.md b/doc/build-windows.md
index ce822dd0dc..9d3ec9cd86 100644
--- a/doc/build-windows.md
+++ b/doc/build-windows.md
@@ -15,6 +15,7 @@ Other options which may work, but which have not been extensively tested are (pl

The instructions below work on Ubuntu and Debian. Make sure the distribution's `g++-mingw-w64-x86-64-posix`
package meets the minimum required `g++` v
...
πŸ’¬ hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3554447029)
> Either way is fine.

We should also bear in mind that the fix for the original issue needs to be backported to 30.x.
⚠️ TheCharlatan opened an issue: "RFC: Add versioning to the kernel header"
(https://github.com/bitcoin/bitcoin/issues/33911)
### Please describe the feature you'd like to see added.

The kernel header installed alongside its dynamic library should be versioned. Versioning best practices around providing a stable API are well established, e.g. through semantic versioning. However the library presents challenges to versioning beyond API stability. Internal features changing, i.e. a new soft fork rule, or a new validation cache, might not have a direct impact on the API, but won't necessarily be desirable to run by all u
...
πŸ’¬ Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-3554637246)
I think this is good for merge?
πŸ’¬ theStack commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3554648564)
@brunoerg @maflcko: Thanks for reviewing, I addressed all the nits.

> FYI I proposed to reword the warning message in #33553 ([7f28483](https://github.com/bitcoin/bitcoin/commit/7f284835be87c4d1a8a56804992043e12dae1ea1)).

Ah nice, makes sense to unify, was already wondering why there are two different messages, each one only mentioning one possible cause.
πŸ’¬ theStack commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3554651834)
Concept ACK
πŸ’¬ TheCharlatan commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3554692728)
Nice, Concept ACK
πŸ’¬ achow101 commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-3554721014)
Finally got around to debugging the actual issue.

The issue has to do with how `std::filesystem::recursive_directory_iterator` handles symlinks. The [docs](https://en.cppreference.com/w/cpp/filesystem/directory_options.html) for the default options that we use say that symlinks are not recursed into for searching. Instead, when we find symlink directories, we look for a wallet.dat file, but we won't search any of the subdirectories.

The problem is that GCC does not implement symlink detect
...
πŸ’¬ hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2543648184)
I meant you could could have this in the intermediate commit:

```C++
NetGroupManager netgroup{std::vector(ASMAP_DATA.begin(), ASMAP_DATA.end())};
```
But then once we reach the commit adding embedding support change to:
```C++
auto netgroup{NetGroupManager::WithEmbeddedAsmap(ASMAP_DATA)};
```
...ending up going directly from `std::array` -> `std::span`.

---

If you prefer the current way I would still avoid `auto` and change:

```diff
- auto asmap_vec{std::vector(ASMAP_DATA.beg
...