💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445611288)
I believe this condition is always true (since it's inside a `while (... && should_shift)`).
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445611288)
I believe this condition is always true (since it's inside a `while (... && should_shift)`).
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445618541)
You're using `curr_selection.back()` several times in the evaluation logic here; would it make sense to have a `auto added_utxo = next_utxo++;` variable in the select logic, and then use `added_utxo` everywhere instead of `curr_selection.back()`? That makes it also clearer that you don't need to test for `curr_selection.empty()`.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445618541)
You're using `curr_selection.back()` several times in the evaluation logic here; would it make sense to have a `auto added_utxo = next_utxo++;` variable in the select logic, and then use `added_utxo` everywhere instead of `curr_selection.back()`? That makes it also clearer that you don't need to test for `curr_selection.empty()`.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445619181)
Since the exploring will always happen (after SHIFT/CUT, possible), perhaps formulate it as "EVALUATE current selection to see whether we can SHIFT or CUT the current selection before EXPLORING further"?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445619181)
Since the exploring will always happen (after SHIFT/CUT, possible), perhaps formulate it as "EVALUATE current selection to see whether we can SHIFT or CUT the current selection before EXPLORING further"?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445622061)
When this loop starts, `should_shift` will always be false, and it can only become true by reaching the `else` branch below. I think it can be simplified to:
```c++
while (next_utxo > 0
...
&& utxo_pool[next_utxo - 1].fee == utxo_pool[next_utxo],fee) {
if (next_utxo >= utxo_pool.size() - 1) {
// Reached end of UTXO pool skipping clones: SHIFT instead
should_shift = true;
break;
}
// Skip clone: previous UTXO is equivalent and unsele
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445622061)
When this loop starts, `should_shift` will always be false, and it can only become true by reaching the `else` branch below. I think it can be simplified to:
```c++
while (next_utxo > 0
...
&& utxo_pool[next_utxo - 1].fee == utxo_pool[next_utxo],fee) {
if (next_utxo >= utxo_pool.size() - 1) {
// Reached end of UTXO pool skipping clones: SHIFT instead
should_shift = true;
break;
}
// Skip clone: previous UTXO is equivalent and unsele
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1882620655)
@mzumsande you're right i lost between branches, sorry.
Apparently caching gives another 12x boost, assuming it works correctly :)
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 35,617.11 | 28,076.40 | 1.9% | 0.01 | `ShouldFanoutTo`
```
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1882620655)
@mzumsande you're right i lost between branches, sorry.
Apparently caching gives another 12x boost, assuming it works correctly :)
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 35,617.11 | 28,076.40 | 1.9% | 0.01 | `ShouldFanoutTo`
```
💬 naumenkogs commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1445793544)
Ahhh I guess I thought it means "as a fallback to the situation from the previous sentence (poor in-place seeds)", but you mean "as a fallback for a general failure" (not that related to the thing above).
This is an even more subtle nit now :)
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1445793544)
Ahhh I guess I thought it means "as a fallback to the situation from the previous sentence (poor in-place seeds)", but you mean "as a fallback for a general failure" (not that related to the thing above).
This is an even more subtle nit now :)
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445804982)
If this is changed, the format string could also be consteval'd into a compile-time checked object to do the newline check (and maybe others), which is currently done by clang-tidy.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445804982)
If this is changed, the format string could also be consteval'd into a compile-time checked object to do the newline check (and maybe others), which is currently done by clang-tidy.
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1882660532)
I'm working on adding a descriptors version of this test so we can have both.
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1882660532)
I'm working on adding a descriptors version of this test so we can have both.
💬 Sjors commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882662121)
It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was _after_ about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a pull request, is also from around [2018](ht
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882662121)
It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was _after_ about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a pull request, is also from around [2018](ht
...
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-1882663651)
Rebased because #27433 needed it.
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-1882663651)
Rebased because #27433 needed it.
🚀 fanquake merged a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172)
(https://github.com/bitcoin/bitcoin/pull/29172)
💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1882738716)
Rebased due to (trivial) merge conflict in test.
I simplified this PR by dropping the debug argument total sigop and other debug info from the RPC result (52e24f662563dbd0adbcfda963052b3999ea689c , 4ef3261eb91c661a0483a730e156a475c2aad14e).
I'll use the Stratum v2 integration #28983 instead to improve debugging tools where needed.
I also dropped the additional check for the sigops limit in 52e24f662563dbd0adbcfda963052b3999ea689c. We've seen in practice that miners sometimes patch temp
...
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1882738716)
Rebased due to (trivial) merge conflict in test.
I simplified this PR by dropping the debug argument total sigop and other debug info from the RPC result (52e24f662563dbd0adbcfda963052b3999ea689c , 4ef3261eb91c661a0483a730e156a475c2aad14e).
I'll use the Stratum v2 integration #28983 instead to improve debugging tools where needed.
I also dropped the additional check for the sigops limit in 52e24f662563dbd0adbcfda963052b3999ea689c. We've seen in practice that miners sometimes patch temp
...
💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1882761201)
Also dropped b175798c67fbe50bc08007fe184d5407a8c4acf4 because the `default_witness_commitment` _is_ absent for empty-block templates.
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1882761201)
Also dropped b175798c67fbe50bc08007fe184d5407a8c4acf4 because the `default_witness_commitment` _is_ absent for empty-block templates.
💬 1440000bytes commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882769175)
- A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
https://x.com/achow101/status/1742427508488729044
https://x.com/achow101/status/1742555598003048753
- There were some misunderstandings about unaddressed issues related to previous pull request:
https://x.com/JeremyRubin/status/1742582215354019956
- Since @reardencode was already working on CHECKSIGFROMSTACK and INTERNALKEY to be used in combination with CHE
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882769175)
- A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
https://x.com/achow101/status/1742427508488729044
https://x.com/achow101/status/1742555598003048753
- There were some misunderstandings about unaddressed issues related to previous pull request:
https://x.com/JeremyRubin/status/1742582215354019956
- Since @reardencode was already working on CHECKSIGFROMSTACK and INTERNALKEY to be used in combination with CHE
...
💬 michaelfolkson commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1882786231)
Concept ACK
A couple of things on first skim read.
1) I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in `wallet_miniscript.py`?
2) Miniscript has always been described by its authors/contributors as an extension of descriptors. I personally wouldn't be against adding some Miniscript explanation, examples in `descriptors.md` but I'd want a short explanation of what Miniscript is making it clear that this decaying multisig examp
...
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1882786231)
Concept ACK
A couple of things on first skim read.
1) I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in `wallet_miniscript.py`?
2) Miniscript has always been described by its authors/contributors as an extension of descriptors. I personally wouldn't be against adding some Miniscript explanation, examples in `descriptors.md` but I'd want a short explanation of what Miniscript is making it clear that this decaying multisig examp
...
👍 fanquake approved a pull request: "build: Fix `-Xclang -internal-isystem` option"
(https://github.com/bitcoin/bitcoin/pull/29195#pullrequestreview-1810865342)
ACK d742be3d3f5d5063d7160f72422bce2fec953f38. The same as what was done in #27328.
> For example, see: https://github.com/llvm/llvm-project/commit/cbbe1d44546db52c71c9a2b18f85b87ae82df9e7
I guess this is meant to be an example of someone else using `-Xclang`? However this commit is from > 10 years ago, and isn't related to, or an explanation for the recent change in Clang behaviour.
(https://github.com/bitcoin/bitcoin/pull/29195#pullrequestreview-1810865342)
ACK d742be3d3f5d5063d7160f72422bce2fec953f38. The same as what was done in #27328.
> For example, see: https://github.com/llvm/llvm-project/commit/cbbe1d44546db52c71c9a2b18f85b87ae82df9e7
I guess this is meant to be an example of someone else using `-Xclang`? However this commit is from > 10 years ago, and isn't related to, or an explanation for the recent change in Clang behaviour.
💬 michaelfolkson commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882816237)
> A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
There is a [pull request](https://github.com/bitcoin/bitcoin/pull/28550) open (in draft) that contains CTV in addition to other opcodes and sighash flags (ie proposed consensus changes).
You don't have to be an expert in combinatorics to realize that continuing down this path results in multiple pull requests to this repo from different authors with different combinations of opco
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882816237)
> A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
There is a [pull request](https://github.com/bitcoin/bitcoin/pull/28550) open (in draft) that contains CTV in addition to other opcodes and sighash flags (ie proposed consensus changes).
You don't have to be an expert in combinatorics to realize that continuing down this path results in multiple pull requests to this repo from different authors with different combinations of opco
...
💬 hebasto commented on pull request "build: Fix `-Xclang -internal-isystem` option":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1882820244)
> I guess this is meant to be an example of someone else using `-Xclang`?
Right, just an example. I forgot about #27328.
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1882820244)
> I guess this is meant to be an example of someone else using `-Xclang`?
Right, just an example. I forgot about #27328.
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1882825009)
I've added another commit which has pretty much the same test but using descriptor wallets i.e. a rescan through `importdescriptors`. Just like the other test, you can reproduce the bug by cherry-picking from #29019. You should get `JSONRPCEXCEPTION: Invalid or non-wallet transaction id (-5)` for the child tx.
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1882825009)
I've added another commit which has pretty much the same test but using descriptor wallets i.e. a rescan through `importdescriptors`. Just like the other test, you can reproduce the bug by cherry-picking from #29019. You should get `JSONRPCEXCEPTION: Invalid or non-wallet transaction id (-5)` for the child tx.
✅ fanquake closed an issue: "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`"
(https://github.com/bitcoin/bitcoin/issues/29174)
(https://github.com/bitcoin/bitcoin/issues/29174)