💬 LarryRuane commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1119061620)
Resetting categories would also touch real code, wouldn't it?
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1119061620)
Resetting categories would also touch real code, wouldn't it?
✅ achow101 closed an issue: "Don't assume signature grinding for external signers"
(https://github.com/bitcoin/bitcoin/issues/26030)
(https://github.com/bitcoin/bitcoin/issues/26030)
🚀 achow101 merged a pull request: "wallet: skip R-value signature grinding for external signers"
(https://github.com/bitcoin/bitcoin/pull/26032)
(https://github.com/bitcoin/bitcoin/pull/26032)
💬 achow101 commented on pull request "[24.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26878#issuecomment-1446809743)
ACK 784a754aa47ce10c6fd99c09cdfc76ee9bc91652
(https://github.com/bitcoin/bitcoin/pull/26878#issuecomment-1446809743)
ACK 784a754aa47ce10c6fd99c09cdfc76ee9bc91652
🚀 glozow merged a pull request: "[24.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26878)
(https://github.com/bitcoin/bitcoin/pull/26878)
💬 pablomartin4btc commented on pull request "cli: add validation to -generate command when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1446919139)
@jonatack thanks for reviewing; as you and @kouloumos recommended and as I commented [here](https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1411046166), I've made the validation more generic for any cli-command moving it up in the code.
As part of this validation, I've added 3 more checks that were not verified before:
- duplication of cli-command (at the moment you can specify -rpcwallet many times, only the last one will be taken into account,
- only 1 cli-command can run at a ti
...
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1446919139)
@jonatack thanks for reviewing; as you and @kouloumos recommended and as I commented [here](https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1411046166), I've made the validation more generic for any cli-command moving it up in the code.
As part of this validation, I've added 3 more checks that were not verified before:
- duplication of cli-command (at the moment you can specify -rpcwallet many times, only the last one will be taken into account,
- only 1 cli-command can run at a ti
...
💬 brunoerg commented on issue "failure in feature_bip68_sequence.py":
(https://github.com/bitcoin/bitcoin/issues/27129#issuecomment-1446919358)
@MarcoFalke I think so, because `send_self_transfer` will call `create_self_transfer` which will call `get_utxo` (since we're not specifying it). `get_utxo` will return the largest utxo and it's usually the coinbase one. However, coinbase transactions needs 100 confs and we're not checking it what may be causing a "bad-txns-premature-spend-of-coinbase" error.
perhaps a fix:
```py
diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py
index 894aff
...
(https://github.com/bitcoin/bitcoin/issues/27129#issuecomment-1446919358)
@MarcoFalke I think so, because `send_self_transfer` will call `create_self_transfer` which will call `get_utxo` (since we're not specifying it). `get_utxo` will return the largest utxo and it's usually the coinbase one. However, coinbase transactions needs 100 confs and we're not checking it what may be causing a "bad-txns-premature-spend-of-coinbase" error.
perhaps a fix:
```py
diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py
index 894aff
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1119204773)
> I'm not sure this is how APS works. When we remove negative UTXOs we assess the whole group and not each UTXO individually, so you don't need to regroup anything after filtering negative groups.
No wait, that would leave negative UTXOs in positive-only groups if the sum of the positive UTXOs surpasses the sum of the negative ones.
Currently, a "positive-only group" is essentially a container who stores only positive UTXOs. Not a mix of UTXOs which values sum is positive. That is differen
...
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1119204773)
> I'm not sure this is how APS works. When we remove negative UTXOs we assess the whole group and not each UTXO individually, so you don't need to regroup anything after filtering negative groups.
No wait, that would leave negative UTXOs in positive-only groups if the sum of the positive UTXOs surpasses the sum of the negative ones.
Currently, a "positive-only group" is essentially a container who stores only positive UTXOs. Not a mix of UTXOs which values sum is positive. That is differen
...
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119107935)
```suggestion
std::optional<std::vector<CTxMemPool::txiter>> CTxMemPool::GetIterVec(const std::vector<uint256>& txids) const
```
Would this be better than giving a special meaning to an empty vector? I'm unsure, but may be worth considering.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119107935)
```suggestion
std::optional<std::vector<CTxMemPool::txiter>> CTxMemPool::GetIterVec(const std::vector<uint256>& txids) const
```
Would this be better than giving a special meaning to an empty vector? I'm unsure, but may be worth considering.
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119109899)
```suggestion
std::optional<std::vector<CTxMemPool::txiter>> CTxMemPool::CalculateCluster(const std::vector<uint256>& txids) const
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119109899)
```suggestion
std::optional<std::vector<CTxMemPool::txiter>> CTxMemPool::CalculateCluster(const std::vector<uint256>& txids) const
```
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119099044)
nit
```suggestion
ret.push_back(it.value());
```
This may help the reader know that `it` is a `std::optional`; at first, I thought `GetIter()` may be returning a pointer. But if you prefer `*`, which is more concise, that's fine too.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119099044)
nit
```suggestion
ret.push_back(it.value());
```
This may help the reader know that `it` is a `std::optional`; at first, I thought `GetIter()` may be returning a pointer. But if you prefer `*`, which is more concise, that's fine too.
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119134160)
Should this function return `std::nullopt` instead?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119134160)
Should this function return `std::nullopt` instead?
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119202243)
I'm concerned about the possible over-estimation here. In theory, there could be a combinatorial explosion if the DAG is highly interconnected and deep. I would consider removing this code, because vector `push_back()` is highly optimized when growth is needed. Or at least make sure there's a problem before adding this optimization.
If you do keep this, consider calling `cluster.shrink_to_fit()` before returning, so at least any high memory usage is temporary.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1119202243)
I'm concerned about the possible over-estimation here. In theory, there could be a combinatorial explosion if the DAG is highly interconnected and deep. I would consider removing this code, because vector `push_back()` is highly optimized when growth is needed. Or at least make sure there's a problem before adding this optimization.
If you do keep this, consider calling `cluster.shrink_to_fit()` before returning, so at least any high memory usage is temporary.
📝 ryanofsky opened a pull request: "refactor: Stop using gArgs global in system.cpp"
(https://github.com/bitcoin/bitcoin/pull/27170)
Most of the code in `util/system.cpp` that was hardcoded to use the global `ArgsManager` instance `gArgs` has been changed to stop using it (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a few hardcoded references to `gArgs` remain. This commit removes the last ones so these functions aren't reading or writing global state.
Noticed these `gArgs` references while reviewing #27073
(https://github.com/bitcoin/bitcoin/pull/27170)
Most of the code in `util/system.cpp` that was hardcoded to use the global `ArgsManager` instance `gArgs` has been changed to stop using it (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a few hardcoded references to `gArgs` remain. This commit removes the last ones so these functions aren't reading or writing global state.
Noticed these `gArgs` references while reviewing #27073
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1119252440)
You're right, thanks for the explanation. This comment could be resolved.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1119252440)
You're right, thanks for the explanation. This comment could be resolved.
💬 Muhammad4599 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1119280534)
_[]()[]()_
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1119280534)
_[]()[]()_
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1447068021)
Updated e8b095f8ad65544f18ad69201d95e11ddf689f1f -> 899ba71e9d7caa370d8a0c4bed2e514f51024dc6 ([removeBlockstorageArgs_0](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_0) -> [removeBlockstorageArgs_1](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_0..removeBlockstorageArgs_1)) with suggested changes.
Rebased 899ba71e9d7caa370d8a0c4bed2e514f51024dc6 -> b640db3c5fbe49af5
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1447068021)
Updated e8b095f8ad65544f18ad69201d95e11ddf689f1f -> 899ba71e9d7caa370d8a0c4bed2e514f51024dc6 ([removeBlockstorageArgs_0](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_0) -> [removeBlockstorageArgs_1](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_0..removeBlockstorageArgs_1)) with suggested changes.
Rebased 899ba71e9d7caa370d8a0c4bed2e514f51024dc6 -> b640db3c5fbe49af5
...
💬 TheCharlatan commented on pull request "refactor: Stop using gArgs global in system.cpp":
(https://github.com/bitcoin/bitcoin/pull/27170#issuecomment-1447112820)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27170#issuecomment-1447112820)
Concept ACK
💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1119334185)
ergonomically
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1119334185)
ergonomically
💬 sipa commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1447126690)
Concept ACK. Migrating to strict JSON-RPC 2.0 for `bitcoin-cli` and whatever opts into 2.0 is cool, even if it means keeping support for kinda-sorta-JSON-RPC 1.0 around on the server side.
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1447126690)
Concept ACK. Migrating to strict JSON-RPC 2.0 for `bitcoin-cli` and whatever opts into 2.0 is cool, even if it means keeping support for kinda-sorta-JSON-RPC 1.0 around on the server side.
💬 satsie commented on issue "Add per message stats to getnettotals rpc":
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1447193880)
Thank you everyone for your input on the concept and design for this issue! I'd like to present the work I have so far for feedback: https://github.com/satsie/bitcoin/pull/4
The PR description has some sample request/responses and summarizes the path I took to get here. I also have some open comments on areas I want to take a closer look at, or am hoping for input on.
Huge thank you to Amiti, Vasil, and AJ for already chiming in on previous iterations of the code :heart:. Since I last comm
...
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1447193880)
Thank you everyone for your input on the concept and design for this issue! I'd like to present the work I have so far for feedback: https://github.com/satsie/bitcoin/pull/4
The PR description has some sample request/responses and summarizes the path I took to get here. I also have some open comments on areas I want to take a closer look at, or am hoping for input on.
Huge thank you to Amiti, Vasil, and AJ for already chiming in on previous iterations of the code :heart:. Since I last comm
...