✅ willcl-ark closed an issue: "Yeah, good point. I'll do a follow-up on this since it's out of scope for the already large change."
(https://github.com/bitcoin/bitcoin/issues/27842)

💬 MarcoFalke commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#issuecomment-1584214045)
looks like the linter failed?

👋 willcl-ark's pull request is ready for review: "init: deduplicate added connections"
(https://github.com/bitcoin/bitcoin/pull/27804)

:lock: fanquake locked an issue: "Yeah, good point. I'll do a follow-up on this since it's out of scope for the already large change."
(https://github.com/bitcoin/bitcoin/issues/27842)

💬 0xB10C commented on pull request "test: handle failed `assert_equal()` assertions in bcc callback functions":
(https://github.com/bitcoin/bitcoin/pull/27831#issuecomment-1584295560)
ACK 4798c4542b44abac008bd0f12642552df35d2cc1. I code-reviewed the changes and they look good to me. The approach taken (applying the two solutions outlined in the OP on a case-by-case basis) seems OK to me.


With https://github.com/bitcoin/bitcoin/issues/27380 in mind, I've checked that this commit https://github.com/0xB10C/bitcoin/commit/4e00fbb8937071fabd3a70c2cc3e6ea8e6f31a65 on top of this PR fails with `AssertionError: not(min relay fee not met == min erlay fee not met)` as expected in
...

💬 MarcoFalke commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1584322983)
jup, 27825 may be easier to play with and get started

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1584327078)
`ff88e08363...5fd41b9fbf`: limit the maximum number of sensitive relay connections that can be opened (discussion: https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218597944)

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1224124709)
I put a cap on the maximum number of sensitive relay connections that can be opened. Breaking through the max open file descriptors looks like a bad idea - it would also hamper access to the filesystem, even if temporarily. I assume somebody may submit thousands of local transactions in no time.

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1584397060)
Added check for IBD

👍 ryanofsky approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1471861464)
Code review ACK f13880e83200ec824d7528061f96708d96bf9bd4

This looks good as is, but would suggest two changes if you feel like making them:

1. Maybe drop the "move 'AbortNode' after 'StartShutdown'" commit if it is not needed anymore. Dropping it would help git blame work better and make history more legible. Also [#27711](https://github.com/bitcoin/bitcoin/pull/27711) is going to remove `AbortNode` so moving it now will cause an uglier conflict.

2. I think it would be better if the mai
...

💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1584534491)
ACK https://github.com/bitcoin/bitcoin/pull/27622/commits/efe5f373e12329104f1c706145eee75c2d2079a7

💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1224295479)
In commit "rpc, wallet: Set blank when disable private keys in createwallet" (37dd054d9c7d58e829338c88df836241c0259f99)

These tests aren't actually being invoked in the current version of the PR. Need to call `test_createwallet` and `test_createwallet_override` functions in `run_test` below

👍 ryanofsky approved a pull request: "wallet: Migrate wallets that are not in a wallet dir"
(https://github.com/bitcoin/bitcoin/pull/26740#pullrequestreview-1472079713)
Code review ACK 52634dba1cba8928b8767d05c8e30b5fd3da45e4, but it would be nice if fix could be simplified as suggested.

💬 ryanofsky commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1224311864)
In commit "wallet: Be able to migrate wallets that are not in a wallet dir" (e22a3a471bf6696cb9ac96f3f348030cb07aa8b2)

This logic seems unnecessarily confusing. I think there is no benefit to writing:

```c++
fs::path db_dir = db_path.parent_path();
if (db_path.filename() == GetName()) {
db_dir = db_path
}
```

Instead of simply:

```c++
const fs::path db_dir = GetName();
```

It also looks like `db_dir` variable is used only once, so you could probably get rid of it and ju
...

💬 ryanofsky commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1224323997)
In commit "test: Add test for migrating default wallet and plain file wallet" (52634dba1cba8928b8767d05c8e30b5fd3da45e4)

Maybe say "legacy top level wallet" instead of "default wallet". Default wallet isn't really a concept that exists after #15454

🤔 furszy reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1472123008)

> 2. I think it would be better if the main() function always returned `node.exit_status` instead conditionally returning it. Better to make it clear that setting `node.exit_status` always determines the exit code and not create doubt about whether the value will be ignored.
> I think this would be a general improvement anyway because it reduces complexity of `AppInit` and make it a pure initialization function, not a function that initializes, interrupts, and waits.

Hmm, isn't the diff e
...

💬 Tmoney91 commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#issuecomment-1584607993)
I need access in my coins and all my Satoshi Nakamoto wallets.



On Fri, Jun 9, 2023 at 8:44 AM Ryan Ofsky ***@***.***> wrote:

> ***@***.**** approved this pull request.
>
> Code review ACK 52634db
> <https://github.com/bitcoin/bitcoin/commit/52634dba1cba8928b8767d05c8e30b5fd3da45e4>,
> but it would be nice if fix could be simplified as suggested.
> ------------------------------
>
> In test/functional/wallet_migration.py
> <https://github.com/bitcoin/bitcoin/pull/26740#discussio
...

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1584647614)
Have quickly drafted the needed changes to make your diff work so we are in the same page, check [61fe1af](https://github.com/bitcoin/bitcoin/pull/27708/commits/61fe1af01418d8b93d38aba8aeb65aecfa6a95cd).

💬 jamesob commented on issue "Stuck chainstate when utxo_snapshot.sh fails":
(https://github.com/bitcoin/bitcoin/issues/27841#issuecomment-1584652993)
Thanks for the report. I think to solve this without a reindex, you just need to run `reconsiderblock 000000000000000000052ac0ce9b8f1a2e7691771e6386f770432c9443dc2af7`. We should probably amend the script to catch failures or interrupts and do this automatically.

🤔 theStack reviewed a pull request: "Remove Sock::Get() and Sock::Sock()"
(https://github.com/bitcoin/bitcoin/pull/26312#pullrequestreview-1472271621)
Concept ACK