Skip to content

rpc: Early call once CWallet::MarkDirty in import calls #14303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
promag wants to merge 2 commits into from
Closed

rpc: Early call once CWallet::MarkDirty in import calls #14303

promag wants to merge 2 commits into from

Conversation

@promag
Copy link
Contributor

@promag promag commented Sep 23, 2018

This gives a small, but not relevant, performance improvement when calling importmulti RPC since MarkDirty is called only once.

Note that ImportScript is not atomic:

bitcoin/src/wallet/rpcdump.cpp

Lines 227 to 242 in 920c090

if (!pwallet->HaveWatchOnly(script) && !pwallet->AddWatchOnly(script, 0 /* nCreateTime */)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
}
if (isRedeemScript) {
const CScriptID id(script);
if (!pwallet->HaveCScript(id) && !pwallet->AddCScript(script)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
}
ImportAddress(pwallet, id, strLabel);
} else {
CTxDestination destination;
if (ExtractDestination(script, destination)) {
pwallet->SetAddressBook(destination, strLabel, "receive");
}
}

ie something can fail after a successful AddWatchOnly, which could lead to invalid cached balances. That's why the MarkDirty call is done as early as possible.

@promag promag mentioned this pull request Sep 23, 2018
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 24, 2018 •
edited
Loading

Reviewers, this pull request conflicts with the following ones:

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.

@promag promag force-pushed the 2018-09-wallet-mark-dirty branch from 7e1cf55 to 0a1bbf7 Compare September 24, 2018 00:54
This was referenced Sep 24, 2018
Merged
Merged
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 28, 2018

Coverage Change (pull 14303) Reference (master)
Lines +0.0339 % 87.0361 %
Functions +0.1390 % 84.1130 %
Branches +0.0039 % 51.5451 %

@meshcollider
Copy link
Contributor

meshcollider commented Oct 8, 2018

Concept ACK

@achow101
Copy link
Member

achow101 commented Oct 9, 2018

utACK 0a1bbf739890408c44dafd96f6643d86d54c2537

@DrahtBot DrahtBot mentioned this pull request Oct 10, 2018
Merged
@meshcollider
Copy link
Contributor

meshcollider commented Oct 18, 2018 •
edited
Loading

utACK 0a1bbf7
Needs rebase

@promag promag force-pushed the 2018-09-wallet-mark-dirty branch from 0a1bbf7 to 852f295 Compare October 20, 2018 10:04
@promag
Copy link
Contributor Author

promag commented Oct 20, 2018

Rebased.

This was referenced Oct 24, 2018
Closed
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 31, 2018

Needs rebase

@promag promag closed this Oct 31, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@promag @DrahtBot @meshcollider @achow101 @laanwj @fanquake