💬 kanzure commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3518043741)
I strongly object to using or appealing to policy to decide on outcomes for this issue. It's the same reason why you should never enter into a street fight: anybody dumb enough to enter a street fight with you is going to be much crazier than you, more heavily armed, less to lose, etc. It's the same thing with policy, you'll just be inviting endless policy argumentation from governancepilled people when in reality our actual goal was to building Bitcoin Core and developing bitcoin, not policy or
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3518043741)
I strongly object to using or appealing to policy to decide on outcomes for this issue. It's the same reason why you should never enter into a street fight: anybody dumb enough to enter a street fight with you is going to be much crazier than you, more heavily armed, less to lose, etc. It's the same thing with policy, you'll just be inviting endless policy argumentation from governancepilled people when in reality our actual goal was to building Bitcoin Core and developing bitcoin, not policy or
...
📝 hebasto opened a pull request: "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it"
(https://github.com/bitcoin/bitcoin/pull/33857)
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593
- https://github.com/bitcoin/bitcoin/pull/33764
The changes in `depends/hosts/mingw32.mk` enable automatic detection of cross-compilers for Windows + UCRT, removing the need to specify them explicitly, as shown
...
(https://github.com/bitcoin/bitcoin/pull/33857)
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593
- https://github.com/bitcoin/bitcoin/pull/33764
The changes in `depends/hosts/mingw32.mk` enable automatic detection of cross-compilers for Windows + UCRT, removing the need to specify them explicitly, as shown
...
🤔 mzumsande reviewed a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854#pullrequestreview-3449234404)
Have you tried this out on signet or mainnet?
I haven't yet, but just from looking at the code I would suspect that we now
1. do the -reindex part without connecting any blocks (except genesis [here](https://github.com/bitcoin/bitcoin/blob/138726a6f8101e0fe7e9ae701ef17b37fcbdee73/src/validation.cpp#L5161))
2. omplete headers-sync with a peer until minchainwork
3. Download the first block we don't have on disk from a peer and only then call `ActivateBestChain()`
4. try to connect that bl
...
(https://github.com/bitcoin/bitcoin/pull/33854#pullrequestreview-3449234404)
Have you tried this out on signet or mainnet?
I haven't yet, but just from looking at the code I would suspect that we now
1. do the -reindex part without connecting any blocks (except genesis [here](https://github.com/bitcoin/bitcoin/blob/138726a6f8101e0fe7e9ae701ef17b37fcbdee73/src/validation.cpp#L5161))
2. omplete headers-sync with a peer until minchainwork
3. Download the first block we don't have on disk from a peer and only then call `ActivateBestChain()`
4. try to connect that bl
...
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518140890)
My Guix build:
```
aarch64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635597430ee32fb26dff8fcd3261f
...
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518140890)
My Guix build:
```
aarch64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635597430ee32fb26dff8fcd3261f
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243)
It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.
Rather than debug (our use of) PyCapnp, I just introduced a `@dataclass` for `CoinbaseTemplateData`, so we fully own it.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243)
It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.
Rather than debug (our use of) PyCapnp, I just introduced a `@dataclass` for `CoinbaseTemplateData`, so we fully own it.
💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514720362)
missed that, yeah, you are right.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514720362)
missed that, yeah, you are right.
💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515093447)
I think this is better to do it in the diff below for two reasons
1. In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](ht
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515093447)
I think this is better to do it in the diff below for two reasons
1. In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](ht
...
💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515020247)
In 91a44988a14037e9c923a789f1b7a616ff902ee1 Stop enforcing descendant size/count limits
After this commit, the `limitdescendantcount` set to 64 in `feature_rbf.py` test is no longer necessary. The `limitancestorcount` set to 64 is also not necessary since we stopped enforcing that in some previous commit.
This can be removed in the followup along with other stale once in feature_dbcrash.py and `wallet_basic.py`.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515020247)
In 91a44988a14037e9c923a789f1b7a616ff902ee1 Stop enforcing descendant size/count limits
After this commit, the `limitdescendantcount` set to 64 in `feature_rbf.py` test is no longer necessary. The `limitancestorcount` set to 64 is also not necessary since we stopped enforcing that in some previous commit.
This can be removed in the followup along with other stale once in feature_dbcrash.py and `wallet_basic.py`.
💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3518166527)
> Can be tested on the following systems:
>
> * Fedora 42 or 43 (requires the `ucrt64-gcc-c++` package).
For example: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/19274168370/job/55109785216.
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3518166527)
> Can be tested on the following systems:
>
> * Fedora 42 or 43 (requires the `ucrt64-gcc-c++` package).
For example: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/19274168370/job/55109785216.
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3518176660)
> > should the depends build instructions be updated to mention this?
>
> I'd prefer updating the docs once `depends/hosts/mingw32.mk` is adjusted so that compilers don't need to be specified explicitly.
Done in https://github.com/bitcoin/bitcoin/pull/33857.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3518176660)
> > should the depends build instructions be updated to mention this?
>
> I'd prefer updating the docs once `depends/hosts/mingw32.mk` is adjusted so that compilers don't need to be specified explicitly.
Done in https://github.com/bitcoin/bitcoin/pull/33857.
💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-3518182486)
> - [ ] Any other documentation / Windows release build configuration updates.
See: https://github.com/bitcoin/bitcoin/pull/33857.
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-3518182486)
> - [ ] Any other documentation / Windows release build configuration updates.
See: https://github.com/bitcoin/bitcoin/pull/33857.
👍 theStack approved a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-3449412250)
ACK 55c6a69f777ac08a14e61b98efea00e5c8f98a5f
With two non-blocking suggestions (or three, including https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443).
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-3449412250)
ACK 55c6a69f777ac08a14e61b98efea00e5c8f98a5f
With two non-blocking suggestions (or three, including https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443).
💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515282136)
follow-up idea: could deduplicate with the `error_message` within `test_backup_during_background_sync_pruned_node`, so it doesn't have to be adapted twice if it ever changes (maybe that was already intended, since you already moved the message into a variable?)
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515282136)
follow-up idea: could deduplicate with the `error_message` within `test_backup_during_background_sync_pruned_node`, so it doesn't have to be adapted twice if it ever changes (maybe that was already intended, since you already moved the message into a variable?)
💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515271970)
nit, for smaller future diff if another node is added
```suggestion
["-fastprune", "-prune=1"],
```
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515271970)
nit, for smaller future diff if another node is added
```suggestion
["-fastprune", "-prune=1"],
```
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518313943)
@glozow thanks for your comments :)
> Can you explain why it's not good practice?
We open the door to other software/users to make easy mistakes by forcing them to do a unit conversion that Core shouldn't have done in the first place. I understand that this has been like this since ¿always?, but giving users the option to have sat/vB (or even more units thanks to FeeFrac) can be useful and reduce the burden on other software.
> It seems extremely dangerous to ever change the default...
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518313943)
@glozow thanks for your comments :)
> Can you explain why it's not good practice?
We open the door to other software/users to make easy mistakes by forcing them to do a unit conversion that Core shouldn't have done in the first place. I understand that this has been like this since ¿always?, but giving users the option to have sat/vB (or even more units thanks to FeeFrac) can be useful and reduce the burden on other software.
> It seems extremely dangerous to ever change the default...
...
💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518350045)
> @plebhash can you try to see if it's easy for you to support both `getCoinbase()` and `getCoinbaseTx()` in [stratum-mining/sv2-apps#59](https://github.com/stratum-mining/sv2-apps/pull/59), preferring the first and falling back to the latter if it doesn't exist?
@Sjors we're not using `getCoinbaseTx` on that code
we do `getBlock` and that provides everything we need, including relevant coinbase info
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518350045)
> @plebhash can you try to see if it's easy for you to support both `getCoinbase()` and `getCoinbaseTx()` in [stratum-mining/sv2-apps#59](https://github.com/stratum-mining/sv2-apps/pull/59), preferring the first and falling back to the latter if it doesn't exist?
@Sjors we're not using `getCoinbaseTx` on that code
we do `getBlock` and that provides everything we need, including relevant coinbase info
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515354081)
It's not clear to me what the plan is, but if the approach isn't switched, the following patch compiles, looks more sensible, and seems to pass all tests:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index ee46f99add8..37015ef69e6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1613,11 +1613,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
AssertLockHeld(m_pool.cs);
auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515354081)
It's not clear to me what the plan is, but if the approach isn't switched, the following patch compiles, looks more sensible, and seems to pass all tests:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index ee46f99add8..37015ef69e6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1613,11 +1613,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
AssertLockHeld(m_pool.cs);
auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED
...
💬 glozow commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518399410)
> forcing them to do a unit conversion that Core shouldn't have done in the first place.
I still don't see the argument for why a unit per kvB is so problematic. BTC and satoshis are easily convertible.
I do think it's worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.
My concern is the idea that the default format might be changed in the future, causing somebody's reading of a feer
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518399410)
> forcing them to do a unit conversion that Core shouldn't have done in the first place.
I still don't see the argument for why a unit per kvB is so problematic. BTC and satoshis are easily convertible.
I do think it's worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.
My concern is the idea that the default format might be changed in the future, causing somebody's reading of a feer
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515659830)
I think @glozow's approach is definitely the right direction to go, but let's defer making that change to a future PR so that it can get more focused review, since it's not trivial to reason about how all this currently works. I'm taking @sipa's suggestion for now and will re-push.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515659830)
I think @glozow's approach is definitely the right direction to go, but let's defer making that change to a future PR so that it can get more focused review, since it's not trivial to reason about how all this currently works. I'm taking @sipa's suggestion for now and will re-push.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515672555)
I took this change in #33591.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515672555)
I took this change in #33591.