refactor: pass mixing wallet to CoinJoin utils by reference#6440
refactor: pass mixing wallet to CoinJoin utils by reference#6440UdjinM6 wants to merge 1 commit intodashpay:developfrom
Conversation
| CTransactionBuilder::CTransactionBuilder(std::shared_ptr<CWallet> pwalletIn, const CompactTallyItem& tallyItemIn) : | ||
| pwallet(pwalletIn), | ||
| dummyReserveDestination(pwalletIn.get()), | ||
| CTransactionBuilder::CTransactionBuilder(CWallet& wallet, const CompactTallyItem& tallyItemIn) : |
There was a problem hiding this comment.
hm, CTransactionBuilder doesn't actually need shared_ptr to wallet, because it doesn't own a copy of wallet in memory... That's a good finding.
There was a problem hiding this comment.
Can we justify this? It doesn't seem correct to me. Are we sure that a CTransactionBuilder object won't outlive the wallet creating it? it seems this is the reason to use shared_ptr no? Please help clarify
There was a problem hiding this comment.
So far as I looked to code, instance of CTransactionBuilder is created in only 2 functions:
- CCoinJoinClientSession::MakeCollateralAmounts
- CCoinJoinClientSession::CreateDenominated
In both cases CCoinJoinClientSession works with CWallet object like with something that definitely exist, because both functions starts from AssertLockHeld(m_wallet.cs_wallet);
And in both cases instance of CTransactionBuilder is a local variable which is removed after usage, so, life of CWallet object is not extended long enough to provide any extra safety. Either CTransactionBuilder is misused and all CJ code is broken (because intensive usage of CWallet everywhere), either no need to keep shared_ptr to CWallet inside CTransactionBuilder at all.
Please, revive this PR, it seems as everything is fine with this changes.
UPD: #6441 seems as gives all good guarantees about CWallet memory ownership during client code
…o prevent concurrent unload 2d7c7f8 fix: do not transfer wallet ownership to CTransactionBuilder{Output} (UdjinM6) 0aeeb85 fix: add missing `AddWallet` call in `TestLoadWallet` (UdjinM6) e800d9d fix: hold wallet shared pointer in CJ Manager/Sessions to prevent concurrent unload (UdjinM6) Pull request description: ## Issue being fixed or feature implemented #6440 (comment) ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 2d7c7f8 Tree-SHA512: 308e3bed077baa2167b7f9d81b87e5a61a113e4d465706548f303dfc499bc072d4e823e85772e591a879986b0fb0413d5afe0e3995e1f939fa772b29adc0300d
…sions to prevent concurrent unload 2d7c7f8 fix: do not transfer wallet ownership to CTransactionBuilder{Output} (UdjinM6) 0aeeb85 fix: add missing `AddWallet` call in `TestLoadWallet` (UdjinM6) e800d9d fix: hold wallet shared pointer in CJ Manager/Sessions to prevent concurrent unload (UdjinM6) Pull request description: ## Issue being fixed or feature implemented dashpay#6440 (comment) ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 2d7c7f8 Tree-SHA512: 308e3bed077baa2167b7f9d81b87e5a61a113e4d465706548f303dfc499bc072d4e823e85772e591a879986b0fb0413d5afe0e3995e1f939fa772b29adc0300d
3 participants
Issue being fixed or feature implemented
As a side-effect, we get rid of
GetWallet()calls in coinjoin module which also fixes a potential deadlock reported recently.What was done?
How Has This Been Tested?
Breaking Changes
Checklist: