The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• tests: Create or use existing properly initialized NodeContexts #20323 (tests: Create or use existing properly initialized NodeContexts by dongcarl)
• addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)
• net: Remove g_relay_txes #20217 (net: Remove g_relay_txes by jnewbery)
• wallet: Avoid locking cs_wallet recursively  #19833 (wallet: Avoid locking cs_wallet recursively by promag)
• Coinstats Index #19521 (Coinstats Index (without UTXO set hash) by fjahr)
• refactor: Get rid of more redundant chain methods #19425 (refactor: Get rid of more redundant chain methods by ryanofsky)
• Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)
• Add address-based index (attempt 4?) #14053 (Add address-based index (attempt 4?) by marcinja)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Simplify patch, make more fuzzing test

#!/bin/bash
while [ 1 ]
do
    src/bitcoin-cli -testnet loadwallet test
    src/bitcoin-cli -testnet unloadwallet test3
    src/bitcoin-cli -testnet loadwallet test2
    src/bitcoin-cli -testnet unloadwallet test
    src/bitcoin-cli -testnet loadwallet test3
    src/bitcoin-cli -testnet unloadwallet test2
done

Use future instead of raw loop for waiting pending callbacks. It still much matters to me compared to #18338

@promag same as your approach with more aggressive refactor + missing virtual desctructors.

Concept ACK, though this is making a lot of changes in a single commit. The changes will probably get simpler if #18338 is merged first, and maybe they can be broken up into smaller commits. Could consider rebasing on top of #18338 of you don't want to wait for it to be merged. I'd also encourage you to review #18338!

Oh, just saw this PR has expanded since I looked at it previously. I think some of the uses of shared_ptr / weak_ptr here like the new ones added in CWalletTx do not make sense. shared_ptr / weak_ptr only make sense when lifetime of the reference doesn't have a definite scope. For cases like CWalletTx where the wallet reference can't outlive the wallet, CWallet& makes more sense than weak_ptr<CWallet>. (Also, though in that specific case, it would be preferable to stop storing the wallet references entirely since they are redundant and no CWalletTx method is ever called without already having a reference to the wallet).

It would probably make sense to break this PR up into multiple commits so individual changes here can be understood and reviewed more easily

shared_ptr / weak_ptr only make sense when lifetime of the reference doesn't have a definite scope.

I agree, in plus it defines no ownership. I agree, that reference makes much more sense here, too

Is this still a bug fix? Do you think you can split in multiple commits?

Hi @promag mostly it's not an issue, AFAIK. It has some side effects in Qt gui, it was fixed i think. Notification proxy can extend notification lifetime which isn't correct to me. As well as using shared pointers only to validation interface, missing virtual destructors to interface classes like NetEventsInterface (it's not a problem since it's not deleted through base pointer till now).
I can split it to more commits if you're interested in this features.

Refactor pwallet to wallet (wallet* to wallet shared_ptr) is removed.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Needs rebase if still relevant

I will investigate if it still needed, actually @ryanofsky does excellent work to well define shared interfaces ownership.

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing for now. Feel free to ask for this to be reopened or create a new pull request.