💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114821225)
If the "fast" (i.e 2s) has been reasoned out in the bip or the paper, the section can be referenced too, I think.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114835166)
Can add a comment about what `TxReconciliationTracker::IsPeerNextToReconcileWith()` does, like assumptions on peers queue and phase we're in.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114866248)
Not sure this new comment still holds with the introduction of `MAX_SET_SIZE=3000`.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114877866)
Correct the max set size (`MAX_SET_SIZE`) is enforced by `ShouldFanoutTo()` which happens before the call to `AddToSet()`, don't know if it could be more conservative with another check here.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114886751)
Well we might have some small discrepancy in old code, doesn't seem to matter here.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114888417)
I think it's accurate with the current comment of warning about less frequent reconciliations introducing high transaction relay latency. Like some trade-off between bandwidth and 0confs UX to be aware, I would say.

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114890070)
Gotcha, just means the malicious outbound peer will never move forward its own reconciliation.

💬 Ayush170-Future commented on issue "listtransactions results order unkown":
(https://github.com/bitcoin/bitcoin/issues/17739#issuecomment-1440726908)
Hello!
Is this issue up for grab? 
Also, if it is, could someone help specify what kind of documentation has to be updated?

I'd love to get started on this.

💬 officialfrancismendoza commented on issue "rpc: prevent non-zero value OP_RETURN outputs in sendrawtransaction":
(https://github.com/bitcoin/bitcoin/issues/25899#issuecomment-1440763838)
I'd like to work on this. Please assign

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1114930512)
hmm, that would make the avoid partial spends flow even uglier, plus I don't think that that will be negligible for big wallets (+50k UTXOs wallets).

The later removal of negative UTXOs will leave APS full groups with less than 100 entries, which will force us to re-order groups so they are full again (moving UTXOs from one group to another), then if the wallet does not include partial groups, will need to discard those groups too.
So making this changes would actually introduce more code th
...

💬 Xekyo commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1110370140)
Here and everywhere in this commit, the `positive_only` check is redundant because `¬A ∨ A ∧ B = ¬A ∨ B`

```suggestion
if (output.GetEffectiveValue() > 0 || !positive_only ) {
```

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1114969240)
yeah k 👌🏼, will do the change if need to re-touch to not invalidate the other reviews only for this small redundancy.

📝 theuni opened a pull request: "Fix various libbitcoinkernel DLL build problems"
(https://github.com/bitcoin/bitcoin/pull/27146)
Fixes #25008. Ping @TheCharlatan

1. Fixup the build defines so that exports are clean.
2. Work around a libtool issue wrt dependency calculation
3. Simplify everything by only ever building in-tree bitcoin-chainstate against a static libbitcoinkernel
4. Remove Windows-only hack that disabled dll creation

👍 ariard approved a pull request: "assumeutxo: background validation completion"
(https://github.com/bitcoin/bitcoin/pull/25740)
ACK a00d0e78.

Since last ACK ab41e8b4:
- move `nPruneTarget` usage to `BlockManager::GetPruneTarget()` in `LoadChainstate()`
- bug report call message (i.e `PACKAGE_BUGREPORT`) in `LogPrintf` in `ActivateBestChain()`
- move `GetUTXOStats` mention to `ComputeUTXOStats` in `MaybeCompleteSnapshotValidation` comment
- new `BOOST_CHECK`s in `chainstatemanager_snapshot_completion` about cache size bytes

💬 mzumsande commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1115048043)
Makes sense - I'll open a small follow-up to address this in a few days.

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1115055900)
Some assertions are not checking that the PR's changes are working correctly, they're ensuring that the test is setting things up as expected, and sometimes that's not obvious to readers. Sorry I didn't think to mention this earlier, but if you retouch, consider adding a comment before these two lines to make it clear that all we're doing here is verifying the test setup.

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1115061429)
I like the way you're calculating the number of blocks needed rather than hard-coding the number; I verified that this value (when I run the test) is 240, which matches what @jonatack suggested.

💬 Mijay36089 commented on issue "Hidden fee (about 15% of sum) while send":
(https://github.com/bitcoin/bitcoin/issues/27120#issuecomment-1440964953)
Where did you send it to what wallet ?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1115299326)
What you described means that the peer clearly has the transaction, thus it can safely be removed from the corresponding set ("try" means it's ok if the tx is not there). What's the problem with that?

💬 S3RK commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#issuecomment-1441341890)
> The downside of this is approach is that I had to sprinkle `const bool can_grind_r = !wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER);` in several different places. @furszy's commit does it in three places. This is fine right now, but at some point we might have some external signers that do support grinding. We'd then have to update the check everywhere.

What if we encapsulate the logic in `Wallet::CanGrindR()`?

The ability to grind signatures is a characteristic of a wallet IIUC
...