๐ค mzumsande reviewed a pull request: "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us"
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3526719032)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
agree with laanwj, not enough trust.
(for the record I still don't view the filtering out newest versions as a major issue, slightly different policies between the different DNS seeds could even be beneficial in some situations)
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3526719032)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
agree with laanwj, not enough trust.
(for the record I still don't view the filtering out newest versions as a major issue, slightly different policies between the different DNS seeds could even be beneficial in some situations)
๐ฌ mzumsande commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3598646376)
> At the least the tx orphanage should not require work when it is empty
That part was merged with #32827
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3598646376)
> At the least the tx orphanage should not require work when it is empty
That part was merged with #32827
๐ฌ laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3598664888)
> So that seems like [compute_builtin_object_size](https://github.com/gcc-mirror/gcc/blob/c9cd41fba9ebd288c4f101e4b99da934bcb96a11/gcc/tree-object-size.cc#L1126) is returning a different value based on the host arch?
Wonder if there's any windows-specific code paths for that in mingw64? After all, we're not seeing it on the other architectures.
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3598664888)
> So that seems like [compute_builtin_object_size](https://github.com/gcc-mirror/gcc/blob/c9cd41fba9ebd288c4f101e4b99da934bcb96a11/gcc/tree-object-size.cc#L1126) is returning a different value based on the host arch?
Wonder if there's any windows-specific code paths for that in mingw64? After all, we're not seeing it on the other architectures.
๐ฌ ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3598680323)
Concept ACK. Would suggest updating `-blockreservedweight` documentation to say it only affects RPC mining clients, not IPC clients.
I think implementation of this could be improved a bit, depending on if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"?
If yes, would suggest just making the field `std::optional`:
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,11 +78,
...
(https://github.com/bitcoin/bitcoin/pull/33965#issuecomment-3598680323)
Concept ACK. Would suggest updating `-blockreservedweight` documentation to say it only affects RPC mining clients, not IPC clients.
I think implementation of this could be improved a bit, depending on if you think it would be useful for IPC clients to "have a way to signal their intent to use the node default"?
If yes, would suggest just making the field `std::optional`:
<details><summary>diff</summary>
<p>
```diff
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,11 +78,
...
๐ hodlinator approved a pull request: "merkle: preโreserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3526806887)
ACK 8af0a3a72c8af781bebed7289657c637220df893
While the benefit of this change to consensus code is low, it's been simplified back to a more easily reviewable version.
The benchmark now has two parts, one that detects mutation issues and one which does not, corresponding to the non-witness & witness merkle root computations performed by Bitcoin Core on mainnet.
### One extra level of paranoia?
I think this change really is simple enough to approve as-is. It's maybe not the same risk-level as
...
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3526806887)
ACK 8af0a3a72c8af781bebed7289657c637220df893
While the benefit of this change to consensus code is low, it's been simplified back to a more easily reviewable version.
The benchmark now has two parts, one that detects mutation issues and one which does not, corresponding to the non-witness & witness merkle root computations performed by Bitcoin Core on mainnet.
### One extra level of paranoia?
I think this change really is simple enough to approve as-is. It's maybe not the same risk-level as
...
๐ฌ hodlinator commented on pull request "merkle: preโreserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2578500589)
Looks good!
nit: With your version of having 2 benchmarks within the same function, `-filter='MerkleRoot.*'` in the commit message can be simplified to `-filter='MerkleRoot'`.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2578500589)
Looks good!
nit: With your version of having 2 benchmarks within the same function, `-filter='MerkleRoot.*'` in the commit message can be simplified to `-filter='MerkleRoot'`.
๐ฌ ryanofsky commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3598781679)
Concept ACK 517a9b23283fee0a861578b2686a71c48a4b67b4, but I had some questions on #33965 which this builds on, and this PR will be affected by what happens to that.
Overall most changes here seem very good: it's nice to introduce a MiningArgs struct and move handling of mining options out of `init.cpp`. Also nice to port more code to use `interfaces::Mining`. Other changes seem less positive. Before, there was a clear separation of which options were controllable by `interfaces/mining.h` and
...
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3598781679)
Concept ACK 517a9b23283fee0a861578b2686a71c48a4b67b4, but I had some questions on #33965 which this builds on, and this PR will be affected by what happens to that.
Overall most changes here seem very good: it's nice to introduce a MiningArgs struct and move handling of mining options out of `init.cpp`. Also nice to port more code to use `interfaces::Mining`. Other changes seem less positive. Before, there was a clear separation of which options were controllable by `interfaces/mining.h` and
...
๐ AfuroIsCrazy opened a pull request: "Update copyright"
(https://github.com/bitcoin/bitcoin/pull/33989)
(https://github.com/bitcoin/bitcoin/pull/33989)
๐ theStack opened a pull request: "test: check that peer's announced starting height is remembered"
(https://github.com/bitcoin/bitcoin/pull/33990)
Note that the announced starting height of a peer is neither verified nor used in any other logic, so reporting a bogus value [1] as done in the test doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages (see also e.g. https://github.com/bitcoin-dot-org/Bitcoin.org/issues/1387#issuecomment-252934859). Admittedly this is not terribly important, but I'd still think having test coverage for this simple reporting functionality is better t
...
(https://github.com/bitcoin/bitcoin/pull/33990)
Note that the announced starting height of a peer is neither verified nor used in any other logic, so reporting a bogus value [1] as done in the test doesn't have any consequences -- it's only used for inspection via the `getpeerinfo` RPC and in some debug messages (see also e.g. https://github.com/bitcoin-dot-org/Bitcoin.org/issues/1387#issuecomment-252934859). Admittedly this is not terribly important, but I'd still think having test coverage for this simple reporting functionality is better t
...
๐ฌ hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3599647251)
> Shouldn't this be removing the other CI (#33764):
>
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Removed.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3599647251)
> Shouldn't this be removing the other CI (#33764):
>
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Removed.
๐ codomposer opened a pull request: "test: Add comprehensive txindex reorg test coverage"
(https://github.com/bitcoin/bitcoin/pull/33991)
# Add comprehensive txindex reorg test coverage
## Motivation
This PR adds a new functional test `feature_txindex_reorg.py` to improve test coverage for the transaction index (`-txindex`) behavior during chain reorganizations. Currently, while txindex functionality is tested in various scenarios, there is limited coverage specifically focused on its behavior during deep reorgs, concurrent access patterns, and edge cases that could affect reliability.
The transaction index is a critical
...
(https://github.com/bitcoin/bitcoin/pull/33991)
# Add comprehensive txindex reorg test coverage
## Motivation
This PR adds a new functional test `feature_txindex_reorg.py` to improve test coverage for the transaction index (`-txindex`) behavior during chain reorganizations. Currently, while txindex functionality is tested in various scenarios, there is limited coverage specifically focused on its behavior during deep reorgs, concurrent access patterns, and edge cases that could affect reliability.
The transaction index is a critical
...
โ
pinheadmz closed a pull request: "test: Add comprehensive txindex reorg test coverage"
(https://github.com/bitcoin/bitcoin/pull/33991)
(https://github.com/bitcoin/bitcoin/pull/33991)
๐ฌ pinheadmz commented on pull request "test: Add comprehensive txindex reorg test coverage":
(https://github.com/bitcoin/bitcoin/pull/33991#issuecomment-3599888466)
Pretty sure this is just AI slop. "Gittensor" is kinda dead give away.
(https://github.com/bitcoin/bitcoin/pull/33991#issuecomment-3599888466)
Pretty sure this is just AI slop. "Gittensor" is kinda dead give away.
โ ๏ธ dachengcheng2022 opened an issue: "tx not exist on the testnet4"
(https://github.com/bitcoin/bitcoin/issues/33992)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
network: testnet4
this is my txinfo: https://mempool.space/zh/testnet4/tx/93cf45c0fb38f62dfde0619ae4abaadc6842a931b6946abd79012d2a963d19e5
tip me confirms already, but i search it use rpc
`curl --location 'http://47.236.168.32:18332' \
--header 'content-type: text/plain;' \
--header 'Authorization: Basic dGVzdDp0ZXN0' \
--data '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawtrans
...
(https://github.com/bitcoin/bitcoin/issues/33992)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
network: testnet4
this is my txinfo: https://mempool.space/zh/testnet4/tx/93cf45c0fb38f62dfde0619ae4abaadc6842a931b6946abd79012d2a963d19e5
tip me confirms already, but i search it use rpc
`curl --location 'http://47.236.168.32:18332' \
--header 'content-type: text/plain;' \
--header 'Authorization: Basic dGVzdDp0ZXN0' \
--data '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawtrans
...
โ
dachengcheng2022 closed an issue: "tx not exist on the testnet4"
(https://github.com/bitcoin/bitcoin/issues/33992)
(https://github.com/bitcoin/bitcoin/issues/33992)
๐ brunoerg opened a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993)
`-stopatheight` is used to stop running bitcoind after reaching a given height. However, this feature is imprecise since some blocks can still be processed during the shutdown.
There are some previous discussions around it in https://github.com/bitcoin/bitcoin/pull/13713, https://github.com/bitcoin/bitcoin/pull/13490 and https://github.com/bitcoin/bitcoin/issues/13477. However, I'm not sure if it will get fixed since it's undesirable to burden the validation code further with this and we can
...
(https://github.com/bitcoin/bitcoin/pull/33993)
`-stopatheight` is used to stop running bitcoind after reaching a given height. However, this feature is imprecise since some blocks can still be processed during the shutdown.
There are some previous discussions around it in https://github.com/bitcoin/bitcoin/pull/13713, https://github.com/bitcoin/bitcoin/pull/13490 and https://github.com/bitcoin/bitcoin/issues/13477. However, I'm not sure if it will get fixed since it's undesirable to burden the validation code further with this and we can
...
๐ฌ l0rinc commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580053828)
Wouldn't it suffice to just hint that it's not exact?
```suggestion
argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the approximate height in the main chain (default: %u). It may be imprecise as shutdown is triggered at this height but additional blocks may be processed.", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
```
nit: if we're doing this, we could adjust the code comment as well:
https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580053828)
Wouldn't it suffice to just hint that it's not exact?
```suggestion
argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the approximate height in the main chain (default: %u). It may be imprecise as shutdown is triggered at this height but additional blocks may be processed.", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
```
nit: if we're doing this, we could adjust the code comment as well:
https://github.com/bitc
...
๐ฌ Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2580229022)
Taken (and updated the test)
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2580229022)
Taken (and updated the test)
๐ฌ brunoerg commented on pull request "init: point out -stopatheight may be imprecise":
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580229771)
Happy to address it if others like it but I personally don't like "approximate" because it's kinda subjective (could mean before or after the height) and do not reflect the goal of it. Also, this issue doesn't happen when it's in IBD.
(https://github.com/bitcoin/bitcoin/pull/33993#discussion_r2580229771)
Happy to address it if others like it but I personally don't like "approximate" because it's kinda subjective (could mean before or after the height) and do not reflect the goal of it. Also, this issue doesn't happen when it's in IBD.
๐ฌ l0rinc commented on pull request "merkle: preโreserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2580278793)
> can be simplified to -filter='MerkleRoot'.
Yeah, thanks, I know, I explicitly extended it since it produces two results now.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2580278793)
> can be simplified to -filter='MerkleRoot'.
Yeah, thanks, I know, I explicitly extended it since it produces two results now.