💬 willcl-ark 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-3517958972)
We opened this issue to give Luke a chance to respond to the concerns in the OP. He has replied but IMO has not addressed the concerns around version filtering.
As documented in this thread, `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is currently not returning any v29 or v30 nodes, which violates point 1 of our DNS seed policy: "The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network." Multiple independent tests have confirmed th
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3517958972)
We opened this issue to give Luke a chance to respond to the concerns in the OP. He has replied but IMO has not addressed the concerns around version filtering.
As documented in this thread, `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is currently not returning any v29 or v30 nodes, which violates point 1 of our DNS seed policy: "The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network." Multiple independent tests have confirmed th
...
📝 yuvicc opened a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856)
## Motivation
This PR refactors `ProcessNewBlock()` and `ProcessNewBlockHeaders()` to return `BlockValidationState` by value instead of using out-parameters or boolean returns. This follows the pattern established in #31981 (commit 74690f4) which refactored `TestBlockValidity()` in a similar manner.
### Current Issues
1. **ProcessNewBlockHeaders()**: Uses an out-parameter `BlockValidationState& state`, making the API less intuitive.
2. **ProcessNewBlock()**: Returns a simple boolean wi
...
(https://github.com/bitcoin/bitcoin/pull/33856)
## Motivation
This PR refactors `ProcessNewBlock()` and `ProcessNewBlockHeaders()` to return `BlockValidationState` by value instead of using out-parameters or boolean returns. This follows the pattern established in #31981 (commit 74690f4) which refactored `TestBlockValidity()` in a similar manner.
### Current Issues
1. **ProcessNewBlockHeaders()**: Uses an out-parameter `BlockValidationState& state`, making the API less intuitive.
2. **ProcessNewBlock()**: Returns a simple boolean wi
...
💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2515053631)
Opened #33856 to address this.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2515053631)
Opened #33856 to address this.
💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3517995745)
From the [issue](https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2388394165) that prompted this:
> I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves
I still think that this is misguided and can get people into "very real issues" if such an option gives the impression that it:
1. Hides the fact that you run a bitcoin node. It does not at all because you will still connect to others at port
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3517995745)
From the [issue](https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2388394165) that prompted this:
> I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves
I still think that this is misguided and can get people into "very real issues" if such an option gives the impression that it:
1. Hides the fact that you run a bitcoin node. It does not at all because you will still connect to others at port
...
💬 fanquake commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518009942)
Guix Build (aarch64):
```bash
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
9e44c7635597430ee32fb26dff8fcd326
...
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518009942)
Guix Build (aarch64):
```bash
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
9e44c7635597430ee32fb26dff8fcd326
...
💬 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...
...