Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 hebasto approved a pull request: "[24.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26878)
ACK 784a754aa47ce10c6fd99c09cdfc76ee9bc91652, I've made backporting locally and got a diff between my branch and this PR as follows:
```diff
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -484,8 +484,8 @@ class ImportDescriptorsTest(BitcoinTestFramework):
assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
change_addr = wmulti_pub.getrawchangeaddress('bech
...
👍1
💬 hebasto commented on pull request "[24.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26878#issuecomment-1446678056)
> ACK [784a754](https://github.com/bitcoin/bitcoin/commit/784a754aa47ce10c6fd99c09cdfc76ee9bc91652)
>
> trivial backports as far as I can see
hebasto closed a pull request: "[24.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26878)
💬 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?
achow101 closed an issue: "Don't assume signature grinding for external signers"
(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)
💬 achow101 commented on pull request "[24.x] Backports":
(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)
💬 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
...
💬 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
...
💬 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
...
💬 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.
💬 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
```
💬 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.
💬 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?
💬 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.
📝 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
💬 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.
💬 Muhammad4599 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1119280534)
_[]()[]()_