💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317023)
Done

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317291)
Done, but it is 34 because the wallet also received 17 previously.

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317498)
Done

💬 epiccurious commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1937358474)
Concept ACK 70171e012a7cd6723aa2b07a3fb3d93920ec2010.

🤔 tdb3 reviewed a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-1874277464)
Excellent work finding a gap and filling it by increasing functional test robustness.
ACK for edc14bc9da332cae39f8803db559b532b3c74e16

Checked out the PR branch, built, and ran all functional tests (all passed).

The review was focused on verifying case coverage of the tests and that tests will accurately catch both pass and fail conditions.

Did not observe previously reported warning message:
```
TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset b
...

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485367696)
I could be missing something, but a test case covering the reset of eviction timeout when an outbound peer sends us a block with a valid longer chainwork (i.e. the timeout set to 0 to prevent eviction, and m_work_header/m_sent_getheaders also reinitialized) was not observed. Do you think it's worth adding?

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485370601)
Added notes below to provide insight into what was reviewed:

If eviction does not occur when it should, then `wait_for_disconnect()` will raise (AssertionError, originally from `wait_until_helper_internal()`). Confirmed by temporarily setting `cur_mock_time` to a value significantly lower than CHAIN_SYNC_TIMEOUT.

Similarly, if unexpected eviction occurs (e.g. in the "keep catching up" case), then `send_and_ping()` or `sync_with_ping()` will raise IOError (`from send_raw_message()`). Conf
...

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485375517)
nit: I could be missing something, but a test case covering the limit of protected outbound peers (e.g. `MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT`) was not observed. Do you think it's worth adding?

Thinking out loud:
`MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4`, so maybe adding another peer that would normally be considered protected (i.e. one that provided a block with the same amount of work as the tip) and seeing that it is not protected could test this?

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485374401)
Added notes below to provide insight into what was reviewed:

In `test_outbound_eviction_protected()` if unexpected eviction occurs (i.e. the P2P connection is no longer present), then `sync_with_ping()` will raise (`IOError('Not connected')`, causing the test to fail. Moved node.disconnect_p2ps() above the last test_node.sync_with_ping() call and, as expected, this occurred.

nit: The heading comment in the `test_outbound_eviction_protected()` omits that a protected peer is also non-block-
...

💬 epiccurious commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1937393546)
utACK 9cf7092474f9b8faaa59fce0a6c26a4df705266c.

💬 epiccurious commented on pull request "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()":
(https://github.com/bitcoin/bitcoin/pull/29355#issuecomment-1937394495)
Concept ACK fa027e08f7be63c201f42d0e06160d2273b4a6dd.

💬 epiccurious commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1937395487)
> code readability would be worsened if I add there const

Seems like adding a const here would be worthwhile. Can you elaborate on the worsened code readability?

💬 epiccurious commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1937396477)
Right now, the test aborts for low disk space after running the Unit Tests for Test Framework Modules.

Instead, does it make sense to abort for low disk space before?

💬 epiccurious commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1485426813)
ACK this nit. Any reason to not include all here?

💬 epiccurious commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1937398262)
> good to include a test case ... only connect to NODE_NETWORK_LIMITED peers when the local chain is within 24h window from the tip

ACK

💬 epiccurious commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1937398480)
utACK d82eafb173d6bfa98a59e86a845013cc8528b65d.

💬 epiccurious commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1937401871)
utACK c6b68c29707770a17617d7d6af572d7460170eab.

> This would be helpful to me

Being able to search through the descriptions of all commands would be helpful.

> This PR allows you to run: `bitcoin-cli help detail`

This approach seems awkward. Is there a better UX option here? What about `bitcoin-cli helpdetail`?

💬 epiccurious commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1937402710)
Concept ACK 5023b47e85161a0b35839ce03c7b5d55ff1cd4c1.

💬 epiccurious commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#issuecomment-1937406162)
utACK 6aae096b7d525e02f54b66b6eb1b4520ae4a7f0c.

💬 epiccurious commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1485441911)
Nit (feel free to disregard) - this is a run-on sentence and complicated to follow. Please break it up to be more readable.