🤔 hebasto reviewed a pull request: "ci: Use native platform for win-cross task"
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3328936219)
> * Unlock the CI task to run on riscv64 at all
Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3328936219)
> * Unlock the CI task to run on riscv64 at all
Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?
👍 hebasto approved a pull request: "ci: Use native platform for win-cross task"
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3329020667)
ACK fa6fd16f36e1240cda58a46e1717b02e8d3172a3, tested on Ubuntu 24.04, RISC-V.
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3329020667)
ACK fa6fd16f36e1240cda58a46e1717b02e8d3172a3, tested on Ubuntu 24.04, RISC-V.
💬 andrewtoth commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2424497697)
We should try and insert a different `coin` at the same `outpoint`, like we do for the `AddCoin` test above. That way the below check `cache.AccessCoin(outpoint) == coin` makes sure the original `coin` emplaced was not overwritten by the second one.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2424497697)
We should try and insert a different `coin` at the same `outpoint`, like we do for the `AddCoin` test above. That way the below check `cache.AccessCoin(outpoint) == coin` makes sure the original `coin` emplaced was not overwritten by the second one.
💬 kevkevinpal commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3394837857)
ACK [faa9d10](https://github.com/bitcoin/bitcoin/pull/33600/commits/faa9d10c84bc6b465cbca266468990cc716b4300)
Looks good to me, makes sense to follow the construct on first use idiom.
I also did a quick grep to see if `g_verify_flag_names` was used anywhere else, and it was not
(https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3394837857)
ACK [faa9d10](https://github.com/bitcoin/bitcoin/pull/33600/commits/faa9d10c84bc6b465cbca266468990cc716b4300)
Looks good to me, makes sense to follow the construct on first use idiom.
I also did a quick grep to see if `g_verify_flag_names` was used anywhere else, and it was not
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2424544149)
Good idea, [done](https://github.com/bitcoin/bitcoin/compare/0d5f7d24bde2bcc2cf7345fa46cb84ac0ecec41f..24d861da7894add47747eff69dd3fc71fbcdd7d0)
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2424544149)
Good idea, [done](https://github.com/bitcoin/bitcoin/compare/0d5f7d24bde2bcc2cf7345fa46cb84ac0ecec41f..24d861da7894add47747eff69dd3fc71fbcdd7d0)
💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3394956274)
> * Is there a way to pass `bitcoin-cli` RPC arguments that begin with a `-` character?
At the moment, please correct me if I'm wrong, there's no or I haven't seen an RPC that would receive a valid argument that starts with `-` (but I understand it could be in the future), and there's no test for it otherwise I think the PR would have caught it.
Testing the scenario you mentioned in `master` would work like for example in:
```
./build_master/bin/bitcoin-cli -regtest -datadir=/tmp/b
...
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3394956274)
> * Is there a way to pass `bitcoin-cli` RPC arguments that begin with a `-` character?
At the moment, please correct me if I'm wrong, there's no or I haven't seen an RPC that would receive a valid argument that starts with `-` (but I understand it could be in the future), and there's no test for it otherwise I think the PR would have caught it.
Testing the scenario you mentioned in `master` would work like for example in:
```
./build_master/bin/bitcoin-cli -regtest -datadir=/tmp/b
...
💬 kevkevinpal commented on pull request "doc: archive release notes for v30.0":
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424672309)
you can drop `kevkevin` I think thats a duplicate for me
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424672309)
you can drop `kevkevin` I think thats a duplicate for me
💬 l0rinc commented on pull request "doc: archive release notes for v30.0":
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424684332)
Like this? :p
```suggestion
- kevin
```
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424684332)
Like this? :p
```suggestion
- kevin
```
💬 kevkevinpal commented on pull request "doc: archive release notes for v30.0":
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424692750)
Haha just `kevkevinpal` works
(https://github.com/bitcoin/bitcoin/pull/33601#discussion_r2424692750)
Haha just `kevkevinpal` works
💬 shyrwall commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3395190327)
Bit late to add this but just saw it because of v30 so just wanted to comment. There are millions upon millions of, mostly chinese, routers out there that only supports UPnP.
For example, basically all of Thailands ISPs use routers from Fiberhome and Huawei. Mostly for PON-deployments. None of these supports anything other than UPnP. That's a single example of tens of millions of connections.
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3395190327)
Bit late to add this but just saw it because of v30 so just wanted to comment. There are millions upon millions of, mostly chinese, routers out there that only supports UPnP.
For example, basically all of Thailands ISPs use routers from Fiberhome and Huawei. Mostly for PON-deployments. None of these supports anything other than UPnP. That's a single example of tens of millions of connections.
💬 kamol1988 commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3395202500)
Yed
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3395202500)
Yed
📝 l0rinc opened a pull request: "[IBD] coins: reduce lookups in dbcache layer propagation"
(https://github.com/bitcoin/bitcoin/pull/33602)
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.
This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all
...
(https://github.com/bitcoin/bitcoin/pull/33602)
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.
This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all
...
🤔 andrewtoth reviewed a pull request: "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`"
(https://github.com/bitcoin/bitcoin/pull/32313#pullrequestreview-3329398843)
ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0
(https://github.com/bitcoin/bitcoin/pull/32313#pullrequestreview-3329398843)
ACK 24d861da7894add47747eff69dd3fc71fbcdd7d0
⚠️ prusnak opened an issue: "Missing shell completions for the `bitcoin` command"
(https://github.com/bitcoin/bitcoin/issues/33603)
### Please describe the feature you'd like to see added.
v30 introduced a new `bitcoin` command but there are no shell completions for this command in contrib/completions/{bash,fish}
it would be great if we had them :)
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
(https://github.com/bitcoin/bitcoin/issues/33603)
### Please describe the feature you'd like to see added.
v30 introduced a new `bitcoin` command but there are no shell completions for this command in contrib/completions/{bash,fish}
it would be great if we had them :)
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
💬 fanquake commented on issue "Missing shell completions for the `bitcoin` command":
(https://github.com/bitcoin/bitcoin/issues/33603#issuecomment-3395305027)
cc @willcl-ark
(https://github.com/bitcoin/bitcoin/issues/33603#issuecomment-3395305027)
cc @willcl-ark
📝 stringintech opened a pull request: "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation"
(https://github.com/bitcoin/bitcoin/pull/33604)
Currently, after assumeutxo background validation finishes, the node continues to skip peers that don't have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.
The restriction persists because `GetSnapshotBaseBlock()` continues to return the snapshot base block even after validation completes. While `m_ibd_chainstate->m_disabled` is set to true when validation fi
...
(https://github.com/bitcoin/bitcoin/pull/33604)
Currently, after assumeutxo background validation finishes, the node continues to skip peers that don't have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.
The restriction persists because `GetSnapshotBaseBlock()` continues to return the snapshot base block even after validation completes. While `m_ibd_chainstate->m_disabled` is set to true when validation fi
...
💬 TheCharlatan commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395334349)
Nice, Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395334349)
Nice, Concept ACK.
💬 fanquake commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395336034)
cc @mzumsande
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395336034)
cc @mzumsande
🤔 l0rinc requested changes to a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-2965376357)
I started a detailed review of this PR and I'm enthusiastic about the concept - it addresses a real problem and the potential performance gains are significant. \
Strong Concept ACK!
However, I've encountered some concerns that make me hesitant to continue reviewing the current implementation in depth:
* The txindex parallelization appears to be broken (showing 3-13% slowdowns rather than speedups), and this went unnoticed for years
* The complexity of the changes makes thorough review very cha
...
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-2965376357)
I started a detailed review of this PR and I'm enthusiastic about the concept - it addresses a real problem and the potential performance gains are significant. \
Strong Concept ACK!
However, I've encountered some concerns that make me hesitant to continue reviewing the current implementation in depth:
* The txindex parallelization appears to be broken (showing 3-13% slowdowns rather than speedups), and this went unnoticed for years
* The complexity of the changes makes thorough review very cha
...
💬 l0rinc commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424838828)
based on my benchmarks on various platforms it seems this isn't speeding up txindex - was this measured by anyone?
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424838828)
based on my benchmarks on various platforms it seems this isn't speeding up txindex - was this measured by anyone?