💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566300445)
have you considered returning early here if `cpfp_candidates` is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566025027)
I think that they are sorted by internal order, not reversed-byte order (because they are sorted by `Wtxid` / `uint256`, not by `uint256.GetHex()`). Is that on purpose? In any case, maybe it would be useful to specify in the doc which order is used to avoid possbile confusion.

Also, this could the tests added in this commit only test for relative order between multiple transactions but not if they are actually sorted in lexicographical order, so that could also be done.

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1565949969)
I asked myself the same question, found this discussion too late. I also think it would be good to mention this in the commit message (so it becomes clear that it has nothing to do with whether `m_replaced_transactions` has entries or is empty).

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566329125)
Could we assume more strictly here that the package size is 2 (for now)?

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566405746)
Why do we only try once in the case there are multiple children in the orphanage, instead of trying multiple times until one package succeeds? To avoid some kind of spamming attacks that could exhaust our computing power?

💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566378431)
Why put the package hash into `m_recent_rejects_reconsiderable` instead of `m_recent_rejects`?
We never reconsider a failed package after all as far as I understand it.

💬 achow101 commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1566440347)
This comment was marked as resolved but nothing was changed.

💬 achow101 commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-2057817221)
Approach NACK

The [review](https://github.com/bitcoin/bitcoin/pull/22693#pullrequestreview-1246558912) I left last year outlines my concerns with this approach, and they have not been addressed in any way.

💬 achow101 commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-2057831468)
ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

💬 achow101 commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2057840155)
Concept ACK

The code looks fine, but I would like to see @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450616033) be addressed.

💬 achow101 commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2057853202)
crACK c941712c269d34e6496db35424e567c7f6ba34e8

Have not yet tested on a Windows machine.

💬 achow101 commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2057879299)
ACK 52cc58ab3ad0ee9df9ccc5303c7f0ce85cf8afee modulo rebase

💬 achow101 commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057925564)
Hmm, the contents of the log files shouldn't matter since the database files have their LSNs reset when the wallet is closed. Is it possible that the wallet files were last opened in an unclean shutdown, or maybe with older versions that didn't always flush on shutdown?

Also, yes, having duplicate files will result in the same unique id which means that the log files can be applied to the wrong file, which will really mess things up.

💬 andrewtoth commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2057954213)
> > Is this related to [bbe82c1](https://github.com/bitcoin/bitcoin/commit/bbe82c116e72ca0638751e063bf564cd1fe5c4d5)?
>
> No, it is related to [0faafb5](https://github.com/bitcoin/bitcoin/commit/0faafb57f8298547949cbc0044ee9e925ed887ba). This PR partially reverts it and documents the flow so the regression does not happen again.

Is it worth it to partially revert? Why not just `git revert 0faafb57f8298547949cbc0044ee9e925ed887ba`? In the happy path it is only locking for a check if there i
...

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058025128)
Updates:
- I think [lint CI failure](https://github.com/bitcoin/bitcoin/pull/28670/checks?check_run_id=20626357772) is unrelated with the code change, it was running successful before started failing with no code change, run `./test/lint/lint-python.py` locally with no failure.

🤔 furszy reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2002419037)
The CI failure is due a merge conflict. Need to rebase the PR. The code you are using is not there anymore in master.

💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2058091748)
> [Expected size of a commitment transaction](https://github.com/lightning/bolts/blob/master/03-transactions.md#expected-weight-of-the-commitment-transaction) is within (900 + 172 * 483 + 224) / 4 = 21050vB

Check BOLT2’s `max_accepted_htlcs`, it’s 483 HTLC outputs in both directions (inbounds / outbounds), so 25k virtual bytes doesn’t work (even if LN implems use more conservative limits in practice, they’re exposed to lightning node operators).

> Would this make things {broken, inconvenie
...

💬 sipa commented on pull request "fuzz: explicitly cap the vsize of RBFs for diagram checks":
(https://github.com/bitcoin/bitcoin/pull/29879#discussion_r1566641643)
Is there a need to abort the fuzz test when this happens (here and below), or could this be a `break` to allow the test to proceed still, without the transaction that would make the total overflow?

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2058142382)
Rebased.

🤔 tdb3 reviewed a pull request: "netbase: clean up Proxy logging"
(https://github.com/bitcoin/bitcoin/pull/29882#pullrequestreview-2002495493)
ACK for d1af4422d13ba84253a1555c2fe5a42170fa8b35
Good cleanup, making logs less cluttered while keeping relevant connection state logging.

In addition to observing the lack of `Cannot connect to socket for` message, also ran a quick test where Tor was stopped then started again. bitcoind was still able to detect and inform the user of the loss of the proxy and subsequent `peer connected` message provided indication that connection through proxy was reestablished.

```
13308 tx=3456746 da
...