👍 jaonoctus approved a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1810195373)
Concept ACK

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-1881989245)
Rebased + fix correct usage of `utf8string()` instead of `u8string()`.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1882055933)
Added missing test title in overview of CoinGrinder tests

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1445523848)
fixed in latest force push [fba275b](https://github.com/bitcoin/bitcoin/pull/28885/commits/fba275b39c83525e98540c110e09d5177fcdb4a0)

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1445557967)
Added a comment ontop showing where I got these numbers from

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1445558060)
updated in 74fae193384b722462a9ede2d55b3375bad74332

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1445559789)
yes you are right updated in [3fbcbc2](https://github.com/bitcoin/bitcoin/pull/28885/commits/3fbcbc26dd2ea0f3257ca52b3f5a037aad7b9b04)

💬 BombayN1 commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1445562578)
Une fois terminer tout ceux qui m'auront aidé seront récompensé

💬 kevkevinpal commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1882243654)
ACK [7f0be89](https://github.com/bitcoin/bitcoin/pull/29156/commits/7f0be8969e721de69fc352f76db5787836803b76)

💬 niteshbalusu11 commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1882399978)
Concept ACK.

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445616402)
I don't think this `std::ceil` is doing anything. The `/` operator is an integer division here, which rounds down. Applying `std::ceil` to that just converts it to a floating point number. I don't think this is incorrect, as rounding down is just more conservative, but it's probably not what you intend.

To compute $\lceil \frac{a}{b} \rceil$, you can use `(a+b-1)/b`.

Also: I don't think `best_selection.empty()` is possible at this point; a UTXO was just added.

💬 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)`).

💬 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()`.

💬 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"?

💬 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
...

💬 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`
```

💬 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 :)

💬 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.

💬 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.

💬 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
...