💬 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

⚠️ Crypt-iQ opened an issue: "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling"
(https://github.com/bitcoin/bitcoin/issues/27843)
- The i2p accept code is in `ThreadI2PAcceptIncoming`, instead of in `ThreadSocketHandler` like the non-i2p accept code.
- When accepting non-i2p connections, a connection is accepted in `AcceptConnection` and if there are more than `maxInbound` outstanding connections, one is marked for disconnect. The next iteration of the loop in `ThreadSocketHandler` will call `DisconnectNodes` and remove it.
- Since the i2p code resides in its own thread, it's possible for i2p connections to stack up pas
...

📝 MarcoFalke opened a pull request: "ci: Use podman stop over podman kill"
(https://github.com/bitcoin/bitcoin/pull/27844)
This should avoid a race where the kill is not done when spinning up the new container. podman stop waits 10 seconds by default.

Fixes https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1217942753

👍 ryanofsky approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1472293444)
Code review ACK 61fe1af01418d8b93d38aba8aeb65aecfa6a95cd. This looks very good.

Appreciate you cleaning up the code to make it more obvious when the new `exit_status` value will and won't be returned. (And sorry I posted another buggy diff that forgot about the early returns!) Left a few more minor suggestions that you could take or leave.

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224398844)
In commit "refactor: decouple early return commands from AppInit" (61fe1af01418d8b93d38aba8aeb65aecfa6a95cd)

Would be nice to just pass `ArgsManager&` to this function instead of `NodeContext& node`, since accessing other parts of the context is not needed.