💬 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
...
(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.
(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
...
(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?
(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.
(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
(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
(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
...
(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
...
(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
...
🤔 fjahr reviewed a pull request: "net: Decouple `CConnman::GetAddresses` from `CNode`"
(https://github.com/bitcoin/bitcoin/pull/33900#pullrequestreview-3484304377)
I'm neutral on this change. I don't think the decoupling is very effective because conceptually the function is still tied to a "requesting peer"/user/attacker looking at the comments around that function. And pushing the access of the `m_network_key` to the call site doesn't really make the API nicer in my view. It would be different if the key was aready available at the call sites alone but that doesn't seem to be the case.
(https://github.com/bitcoin/bitcoin/pull/33900#pullrequestreview-3484304377)
I'm neutral on this change. I don't think the decoupling is very effective because conceptually the function is still tied to a "requesting peer"/user/attacker looking at the comments around that function. And pushing the access of the `m_network_key` to the call site doesn't really make the API nicer in my view. It would be different if the key was aready available at the call sites alone but that doesn't seem to be the case.
💬 fjahr commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543218680)
I think the comment is slightly less helpful with the current version and the renaming of the variable otherwise.
```suggestion
* @param[in] network_key Network key of the requesting peer. Used to key the cache to prevent privacy leaks.
```
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543218680)
I think the comment is slightly less helpful with the current version and the renaming of the variable otherwise.
```suggestion
* @param[in] network_key Network key of the requesting peer. Used to key the cache to prevent privacy leaks.
```
💬 fjahr commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543220778)
Alternatively the variable could be named `requestor_key` or something similar to transport this information.
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543220778)
Alternatively the variable could be named `requestor_key` or something similar to transport this information.
💬 achow101 commented on pull request "wallet: refactor ProcessDescriptorImport":
(https://github.com/bitcoin/bitcoin/pull/33874#discussion_r2543714661)
This is dangerous, we should never return `success: true` if the descriptor hasn't been fully processed yet.
(https://github.com/bitcoin/bitcoin/pull/33874#discussion_r2543714661)
This is dangerous, we should never return `success: true` if the descriptor hasn't been fully processed yet.
💬 achow101 commented on pull request "wallet: refactor ProcessDescriptorImport":
(https://github.com/bitcoin/bitcoin/pull/33874#issuecomment-3554828252)
Concept meh
`importdescriptors` was modeled after the old `importmulti`, which IIRC part of the point was that even if some things couldn't be imported that everything else that can be imported was.
But I do see how exiting early and doing nothing if something can't be imported could be useful, but this is pretty big semantic change and we need to be careful about return values to avoid any possible confusion.
(https://github.com/bitcoin/bitcoin/pull/33874#issuecomment-3554828252)
Concept meh
`importdescriptors` was modeled after the old `importmulti`, which IIRC part of the point was that even if some things couldn't be imported that everything else that can be imported was.
But I do see how exiting early and doing nothing if something can't be imported could be useful, but this is pretty big semantic change and we need to be careful about return values to avoid any possible confusion.
💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3554829049)
> But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that -asmap=1 would load a file called 1 if we don't take additional steps to prevent this.
Yes, this is what has bothered me most about the arg for a long time and why I had included my minimal fix for this in the original embedding PR already, just implemented a bit differently than you suggest but with the same effect. I think it's just not acceptable that you couldn't even use the
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3554829049)
> But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that -asmap=1 would load a file called 1 if we don't take additional steps to prevent this.
Yes, this is what has bothered me most about the arg for a long time and why I had included my minimal fix for this in the original embedding PR already, just implemented a bit differently than you suggest but with the same effect. I think it's just not acceptable that you couldn't even use the
...
🤔 mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3484869685)
ACK c5b301f6ac0d3f882cc55397e3fb0cf95058704b
Fun fact: The 6-block warning is very old, introduced by Satoshi in https://github.com/bitcoin/bitcoin/commit/94cfec07fd302c9ff9b6a80c47418d4fe56596ae
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3484869685)
ACK c5b301f6ac0d3f882cc55397e3fb0cf95058704b
Fun fact: The 6-block warning is very old, introduced by Satoshi in https://github.com/bitcoin/bitcoin/commit/94cfec07fd302c9ff9b6a80c47418d4fe56596ae
💬 mzumsande commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655)
nit: isn't "at least" imprecise, and you need _more than_ 6 blocks?
The validation codes uses `>` instead of `>=` and the test fails if I create just 6 blocks on top of the tip.
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655)
nit: isn't "at least" imprecise, and you need _more than_ 6 blocks?
The validation codes uses `>` instead of `>=` and the test fails if I create just 6 blocks on top of the tip.
🤔 hebasto reviewed a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3484940469)
Maybe update `xcb_proto` as well?
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3484940469)
Maybe update `xcb_proto` as well?
💬 hebasto commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2543728983)
Why not [1.17](https://gitlab.freedesktop.org/xorg/lib/libxcb/-/commits/libxcb-1.17.0?ref_type=tags)?
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2543728983)
Why not [1.17](https://gitlab.freedesktop.org/xorg/lib/libxcb/-/commits/libxcb-1.17.0?ref_type=tags)?
💬 achow101 commented on pull request "wallet: Add separate balance info for non-mempool wallet txs":
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3554858459)
Concept NACK-ish
There's a lot of reasons that an unconfirmed transaction might be in the wallet but not the mempool, and I think giving an accurate balance of those transactions will be hard to get right. We absolutely do not want to mislead users into thinking they have more coins than they actually do, so I am hesitant to add something like this.
From a cursory glance at the code, I think 2 transactions that conflict with each other but aren't in the mempool would both get counted towar
...
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3554858459)
Concept NACK-ish
There's a lot of reasons that an unconfirmed transaction might be in the wallet but not the mempool, and I think giving an accurate balance of those transactions will be hard to get right. We absolutely do not want to mislead users into thinking they have more coins than they actually do, so I am hesitant to add something like this.
From a cursory glance at the code, I think 2 transactions that conflict with each other but aren't in the mempool would both get counted towar
...