🚀 achow101 merged a pull request: "rpc: use CScheduler for relocking wallet and remove RPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32862)

💬 pablomartin4btc commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2191271514)
I don’t have a strong preference either way, but I agree, the loading failure is part of the broader migration process. Also, if we (ever) later refactor the migration logic into a separate component, the current comment might no longer reflect the actual flow, making it prone to becoming outdated.

💬 HowHsu commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#discussion_r2191340453)
I tend to favor the principle of adding things only when they're actually needed.

💬 ryanofsky commented on pull request "POC: IPC tracing interface":
(https://github.com/bitcoin/bitcoin/pull/32898#issuecomment-3047160262)
Notes on current implementation of this:

- The third commit f880817d54d9558938c697a37d0ef9acd4f34cf4 would be a lot simpler if it added a `Traces* g_traces` or similar global variable, which would be a reasonable thing to do. Initially, I wanted to avoid globals because they are hard to get rid of, and avoiding them could have benefits like being able to attach different tracers to different `CCoinsViewCache` instances, but this may not be worth extra complexity.
- In the main commit 8e09fbe
...

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3047190022)
> furszy

But like furszy said, the Sync() in test uses mutex and conditional variables to sync with the main thread, while the real Sync() in bitcoind changes as times goes by.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191468854)
Done! Added you as co-author on that commit.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3047392949)
<ins>_Updates_</ins>:
- Addressed feedback from @maflcko (co-authored): extended `MaybeArg` to return a `std::optional<std::string>` for use in the `GetWalletNameFromJSONRPCRequest` overload. Also refactored the `migratewallet` handling of `wallet_name` to align with the existing behavior in `unloadwallet`.
- Incorporated @furszy's suggestion (co-authored): added a new template in RPC utils to check whether all provided parameters are null or empty, now used in the `getdescriptoractivity` RPC.
...

💬 sean-k1 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3047454052)
@achow101 @maflcko
PTAL

💬 l0rinc commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3047465141)
@pstratem, not sure if you saw this, but could be helpful: https://github.com/bitcoin/bitcoin/pull/25081

💬 1440000bytes commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3047520707)
> The test should probably be added to the PR after the commit fixing it

I agree

💬 1440000bytes commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3047522544)
> I don't think the test is useful.
>
> Other than that, concept ACK.

You can ignore it.

💬 stratospher commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3047561903)
> In InvalidateBlock, both the block failure flags and m_best_header are kept up to date with each block that is disconnected, so it's not necessary to recalculate them at the end of this function.

maybe I'm missing something but even without the `calc_flags_and_header=false` flag, this means that [the condition](https://github.com/bitcoin/bitcoin/blob/a8bff38236ac988f82dcfa85438946cfe0d3afe3/src/validation.cpp#L2074) cannot hit true and `RecalculateBestHeader()` in `InvalidChainFound()` cann
...

💬 Sjors commented on pull request "rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt":
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3047642328)
@glozow thanks, I hadn't seen that one. Will take a look.

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3047748032)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f

@achow101 can you make a detached signature for either macOS or Windows for the current commit? It seems like a good precaution to test that given: https://github.com/bitcoin/bitcoin/commit/03f585d58ccf4d5c02d621c5b6046d45807b3201

(I'll link to my non code-signed guix build when it finishes)

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2191742180)
Looks great with `mdcat` :-)

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3047771879)
Linter is unhappy over an `unused import` in the test.

👍 stratospher approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2996421512)
reACK 8cc3ac6.

💬 maflcko commented on pull request "ci: Catch tests corrupting the source directory":
(https://github.com/bitcoin/bitcoin/pull/32874#issuecomment-3047791077)
> Better not to trust CI to do a security audit for you...

Yeah, anything in the CI is more a belt-and-suspenders and a way to catch "sunny-day" violations.

Generally, the approach of removing write access can only catch unintentional and accidental violations anyway, because an intentional violation could just guard itself when a write error happens due to missing access. So any violation would have to directly lead to the process being killed, or otherwise logged to mark the tests as fai
...

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500)
why is this needed? A pointer can already hold the state of null, so i don't see why the translation to optional is needed

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191840823)
this just makes it confusing to special-case string here and doesn't seem ideal having to create a full copy of the possibly long string for no reason