Improve wallet atomicity. #339
Conversation
Reorganize that code around to prevent a deadlock due to mixing locking orders. Since this mechanism has some issues, unexport it while here so other packages don't make the same mistake.
|
tested thoroughly on simnet. Seeing huge increase in wallet processing. Though there still appears to be an issue with address pool and db getting out of sync under heavy load during large amounts of autopurchasing (--enablestakemining --ticketmaxprice=20 --ticketbuyfreq=20) |
|
I ran this on two testnet nodes (a bitrig one that used dcrticketbuyer and an osx one that just purchased on its own) each connected to a daemon on decred/dcrd#349 (the new ticketdb) with no issues. |
…aster do not error from connectBlock if auto ticket purchasing fails.
…lock, when pruning.
|
I just tried a restore from seed on testnet. Took under 20 minutes on a fairly slow machine to rescan for 10888 addresses so not bad. All seems to be working after that. |
|
Appears to have all the bugs from simnet testing shaken out. tACK |
|
Switched over 2 of my 3 testnet stakepool wallets to this and they are voting/noticing/accepting new stake tickets OK. They're also staying in sync with the non-upgraded node. Will update if anything goes off the rails. tACK |
| } | ||
|
|
||
| _, err = w.NextAccount(cmd.Account) | ||
| _, err := w.NextAccount(cmd.Account) |
There was a problem hiding this comment.
You check below for if waddrmgr.IsError(err, waddrmgr.ErrLocked) { ... }, but if the error is of a different type it appears to be unhandled and overwritten.
There was a problem hiding this comment.
you are correct. don't think I need to say it but I am not a fan of this error model.
There was a problem hiding this comment.
Well, that's not totally true. If it's non-nil but not waddrmgr.ErrLocked, then it will still be returned with whatever the wallet fallback error code is.
| @@ -1115,21 +1057,16 @@ func getNewAddress(icmd interface{}, w *wallet.Wallet) (interface{}, error) { | |||
| toReturn := make(map[string]string) | |||
| toReturn["address"] = addr.EncodeAddress() | |||
|
|
|||
| ainfo, err := w.Manager.Address(addr) | |||
| pubKey, err := w.PubKeyForAddress(addr) | |||
There was a problem hiding this comment.
It looks like you always just create a pubkey address from the pubkeys after fetching them. Why not just have a function that returns the pubkey address?
There was a problem hiding this comment.
Either can work. But if this code isn't wrong in some way, I don't see the point in changing it.
| Height: 0, | ||
| } | ||
|
|
||
| addrInfo, err := w.Manager.ImportScript(addrmgrNs, script, bs) |
There was a problem hiding this comment.
If ImportScript fails here due to the wallet being locked, the database update transaction where the transaction script is written to the wtxmgr will also fall through. I think the legacy methods allowed it to be written separately, but you may consider that to be a bug.
There was a problem hiding this comment.
I can't think of any reason why you would want it added in one and not the other. Not changing this.
| pkScript, err := txscript.PayToAddrScript(addr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot create txout script: %s", err) | ||
| } | ||
| msgTx.AddTxOut(wire.NewTxOut(int64(toReceive), pkScript)) | ||
|
|
||
| // Start creating the SignRawTransactionCmd. | ||
| outpointScript, err := txscript.PayToScriptHashScript(msCredit.ScriptHash[:]) | ||
| outpointScript, err := txscript.PayToScriptHashScript(p2shOutput.P2SHAddress.Hash160()[:]) |
There was a problem hiding this comment.
Why Hash160 instead of ScriptHash? I think they are the same thing, though.
There was a problem hiding this comment.
same thing, no reason to change it.
|
|
||
| // Defined OutputKind constants | ||
| const ( | ||
| OutputKindNormal OutputKind = iota |
| // Add the address to the notifications watcher. | ||
| addrs := make([]dcrutil.Address, 1) | ||
| addrs[0] = curAddress | ||
| if err := chainClient.NotifyReceived(addrs); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| a.cursor++ |
There was a problem hiding this comment.
This changes the functionality, but is correct. OK
| const ( | ||
| OutputKindNormal OutputKind = iota | ||
| OutputKindCoinbase | ||
| OutputKindStakebase // not returned by all APIs yet |
There was a problem hiding this comment.
There are 4x kinds of special stake outputs: OP_SSTX tagged (ticket voting rights), OP_SSGEN tagged (voting subsidy/stakebase), OP_SSRTX tagged (missed and revoked tickets), and OP_SSTXCHANGE (ticket change). Might want to have these instead.
There was a problem hiding this comment.
i'm still fuzzy on the decred details here but the idea was that stakebase would be all regular tx tree coins generated due to participating in voting. I'm not sure if this type needs to represent outputs in the stake tree.
This is a step towards completing #315.
This changes the database access APIs and each of the "manager" packages (waddrmgr/wtxmgr/wstakemgr) so that transactions are opened (only) by the wallet package and the namespace buckets that each manager expects to operate on are passed in as parameters.
This helps improve the atomicity situation as it means that many calls to these APIs can be grouped together into a single database transaction.
This change does not attempt to completely fix the "half-processed" block problem. Mined transactions are still added to the wallet database under their own database transaction as this is how they are notified by the consensus JSON-RPC server (as loose transactions, without the rest of the block that contains them). It will make updating to a fixed notification model significantly easier, as the same "manager" APIs can still be used, but grouped into a single atomic transaction.
This PR requires massive testing. Nearly all parts of wallet are at least indirectly affected by this change. I can not guarantee that this will not result in a deadlock due to the changes to where transactions are opened (e.g. code inside of a write transaction may attempt to open another transaction).