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

💬 epiccurious commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1485441251)
Why remove this?

💬 epiccurious commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1937407280)
Concept ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.

> Fix all issues by removing it.

Any conceivable downsides to removing it?

💬 epiccurious commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1937408052)
> Changes all uses of LogPrintLevel() where the level is hardcoded, and changes all LogPrintf and LogPrint uses in init.cpp.

Trying to understand the concept here. What problem does this solve?

💬 epiccurious commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-1937408907)
I'd like to reproduce your issue on my local environment.

Can you please provide steps to reproduce this issue?

Also, does this issue happen every time, or is it intermittent?

💬 epiccurious commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1937413112)
> points to a "private list of security officers" where no member is explicitly named

In light of this, @ariard are you comfortable closing the issue? If not, what specific objection?

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1937431126)
> I think a good approach would avoid adding a TxStateMempoolConflicted top-level state, and just add a bool mempool_conflicts or std::set<Txid> mempool_conflicts member to the TxStateInactive struct.

Thanks for your feedback, I have implemented this.