💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610031890)
> The debug logs could be added when needed. There are already two log lines, so a third (when added) shouldn't matter too much, I'd say.
>
> For example:
>
> ```
> node1 2024-05-01T17:15:16.401946Z (mocktime: 2024-05-01T16:46:25Z) [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getpeerinfo user=__cookie__
> node1 2024-05-01T17:15:16.453092Z (mocktime: 2024-05-01T16:46:25Z) [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / fro
...

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610033578)
> I'm not sure we should rely on the `pong` alone because that might change over time?

If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

See the next line comment:

```
# The message bytes are counted before processing the message, so make
# sure it was fully processed by waiting for a ping.

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610039510)
Yeah, sounds good to close this thread for now.

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1606940558)
nit `TxRequest` isn't a thing?

🤔 instagibbs reviewed a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2066488661)
looking pretty straightforward but I'm not an expert in the current locking setup

will give another pass in a bit

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607161194)
not a locking expert but would `LOCKS_EXCLUDED` be easier to read?

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607169815)
in commit message mention what it's obsoleted by, e.g., external mutex by caller

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607177017)
what's the value of having the now bare wrapper `EraseTx`

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607156264)
seems to be covered at https://github.com/bitcoin/bitcoin/pull/30111/commits/9d413afd127ab1753f316d5d3c54e74830f3c9b6#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R3635 now

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610060323)
fixed, thanks

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610060814)
took, thanks

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2124935777)
> I've noticed two issues in the "tidy" job so far:
>
> 1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
>
> 2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.

The recent push includes:
- commits from https://github.com/hebasto/bitcoin/pull/205, which fixes the point 1
- commits from https://github.com/hebasto/bitcoin/pull/206, which fixes the point 2

🤔 furszy reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2071420549)
Thanks for the review ryanofsky!

> I just noticed [bitcoin/bitcoin#25269](https://github.com/bitcoin/bitcoin/pull/25269), which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment. (Though maybe that PR is still a slight regression because it doesn't include the amount of the fee in the error message.) I wonder if you'd consider reopening that PR and maybe includin
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610124181)
Added in b2322fc476dc983b91492768d2054c2a3966afdd

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610124223)
Thanks, fixed in 0f4ddfa43749266960a5d05693ed0cdc993328d6.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1610144238)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 2e29e9beeebcadfc7afc56a28791456d40551bed: I think `MAX_SET_SIZE` could be in `txreconciliation.h` then we can use it in the tests.

```diff
-/**
- * Maximum number of wtxids stored in a peer local set, bounded to protect the memory use of
- * reconciliation sets and short ids mappings, and CPU used for sketch computation.
- */
-constexpr size_t MAX_SET_SIZE = 3000;

/**
* Maximum number of transactions for which we
...

🤔 furszy reviewed a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2071488793)
Code review ACK 078c2d74c2779dd459e42daf67ab450fbf223506. Nothing blocking, just tiny nits.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610134627)
Non-blocking because it is the only way now but it is kinda unnatural to provide `batch` and `w_batch` when `w_batch` contains the `batch` object.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610135840)
nit: should check `WriteKeyMetadata` return value.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1610125269)
nit: `!updated_metas.empty()`