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

💬 Sjors commented on pull request "Remove Taproot activation height":
(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)

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

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

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