multi: Update to latest dcrd/dcrwallet dev modules #953
Conversation
server/asset/dcr/dcr.go
Outdated
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("failed to decode MsgTx from hex for transaction %s: %w", txHash, err) | ||
| } | ||
| txTree := stake.DetermineTxType(msgTx, msgTx.Version == wire.TxVersionTreasury) |
There was a problem hiding this comment.
Kind of makes me wonder why DetermineTxType doesn't just check the version itself.
There was a problem hiding this comment.
Right. Gonna ping @davecgh for this.
There was a problem hiding this comment.
It depends on the result of the treasury vote. This only works here because dcrd is actually enforcing the condition is true, which it couldn't do if it were relying on the very condition it has to enforce.
There was a problem hiding this comment.
As an aside, this code does not look accurate. DetermineTxType doesn't return the txTree, it returns the transaction type txType, so it's passing the wrong thing to getUnspentTxOut, which is why you're having to cast it too.
isTreasuryEnabled := msgTx.Version == wire.TxVersionTreasury
txType := stake.DetermineTxType(msgTx, isTreasuryEnabled)
tree := wire.TxTreeRegular
if txType != stake.TxTypeRegular {
tree = wire.TxTreeStake
}
txOut, pkScript, err := dcr.getUnspentTxOut(ctx, txHash, vout, tree)EDIT: I modified that a bit to (hopefully) make it more clear what the code is doing.
There was a problem hiding this comment.
Also, I didn't look at the surrounding code, but that msgTx.Version == wire.TxVersionTreasury is only guaranteed to be accurate for the first transaction in the stake tree. You can't just look at any old transaction's version and conclude that.
There was a problem hiding this comment.
I see that bad code now @davecgh. I mean the function says determine txtype not tree!
Re the msgTx.Version == wire.TxVersionTreasury check guarantee, it came up in chat a while ago (about the time the tree param was added to the gettxout method) and it was suggested that this check is sufficient to tell if treasury is enabled. If it's not sufficient and we do not have access to the full block to pull the first stake tx, is there a better alternative? Or simply just true as is done in a couple other call sites I've seen?
There was a problem hiding this comment.
Oh, I see you just said that this shortcut isn't even valid for arbitrary txns anyway, only the first in the stake tree.
Well, can we always say
truethen? If it is actuallyfalse, then there won't be any treasury txns or new vote versions since dcrd wouldn't allow them, right? So in this casetruewould just be inefficient, but it wouldn't falsely identify certain regular transactions as stake.
It depends on exactly how it's being used. The only things that changed about the old transactions are:
- Regular tree coinbase transactions
- Stake tree votes
All of the new treasury transactions will obviously be prevented before the vote passes by dcrd, so you really wouldn't have any issue there in terms of just using true.
However, you will have an issue with it detecting current coinbases and votes if you pass it true prior to treasury activation because their format changed.
There was a problem hiding this comment.
It's not the prettiest, but a potential solution is to just check both and if neither come up as a stake transaction, you'll be fine, because dcrd will prevent the wrong ones from making it into a block accordingly.
e.g. something like:
func determineTxTree(msgTx *wire.MsgTx) int8 {
// Try with treasury disabled first.
txType := stake.DetermineTxType(msgTx, false)
if txType != stake.TxTypeRegular {
return wire.TxTreeStake
}
// Try with treasury enabled.
txType = stake.DetermineTxType(msgTx, true)
if txType != stake.TxTypeRegular {
return wire.TxTreeStake
}
return wire.TxTreeRegular
}
...
tree := determineTxTree(msgTx)
txOut, pkScript, err := dcr.getUnspentTxOut(ctx, txHash, vout, tree)
There was a problem hiding this comment.
Thanks for bearing with me @davecgh. I want to get clear on this for dcrdata as well.
All of the new treasury transactions will obviously be prevented before the vote passes by dcrd, so you really wouldn't have any issue there in terms of just using
true.However, you will have an issue with it detecting current coinbases and votes if you pass it
trueprior to treasury activation because their format changed.
For coinbases, I have noted that blockchain/standalone.IsCoinBaseTx has an issue if we say true prematurely:
However, for stake.DetermineTxType, it appears that a coinbase would just remain in the default case (TxTypeRegular) after (still) matching none of the stake types regardless of isTreasuryEnabled.
For votes, that's a question of what stake.CheckSSGenVotes will do. In this case it seems that saying true incorrectly/prematurely will also not be a problem, but I'm less certain about this:
Saying true with stake.CheckSSGenVotes appears to have no effect for old votes whose last output is not a nulldata output.
There was a problem hiding this comment.
Yeah, I think you can be a little loose in dcrdata with the vote check and just use true for that since dcrd isn't going to let bad votes in and it appears old votes will still be identified when given true. I actually thought that wasn't the case, but after looking at it again, I see that it will because the final treasury spend bits are allowed to be omitted.
So, even though it's not technically accurate. since dcrdata is only ever looking at votes in the mempool (which comes from dcrd via RPC, right?) and blocks which have been validated by dcrd, dcrdata should be protected from misidentifying them.
For the coinbase case, since they aren't allowed in the mempool, you could get away with only checking with and without treasury enabled as I suggested above for the first tx of the regular tree and that would also always be correct since dcrd will prevent the wrong types from making it into a block.
client/asset/dcr/dcr.go
Outdated
| return &auditInfo{ | ||
| output: newOutput(txHash, vout, toAtoms(txOut.Value), wire.TxTreeRegular), | ||
| output: newOutput(txHash, vout, toAtoms(txOut.Value), txTree), |
There was a problem hiding this comment.
If we're sure the tree is always going to be wire.TxTreeRegular or we want to enforce that for contracts we audit, then instead of checking both trees in the gettxout call above, we can modify that to only check the regular tree and undo this change.
This is what is done for Refund. We only check the regular tree and when creating the prevout, we hardcode wire.TxTreeRegular. But that's likely only safe because we're refunding contracts we created which are currently all regular tree outputs.
There was a problem hiding this comment.
Are there any circumstances under which this pubkey script would be accepted in a valid stake tree transaction? It's not tagged with a stake opcode, and I don't think it will validate as a valid output in a ticket, vote, or revocation.
There was a problem hiding this comment.
It's possible, yes. Discussed this with @davecgh on dex chat. Spendable outputs in the stake tree "can be a pay-to-script-hash which could theoretically be anything".
It is possible albeit extremely unlikely for a contract output to be a vote or revocation output i.e. if the user purchased a ticket with a commitment to the contract p2sh address. Another possibility is if a contractor submitted their invoice with an address which itself was actually just funding a swap, i.e. after the treasury vote activates - the payment to that address would be a stake tree tx.
| // Could check with gettransaction first, figure out the tree, and look for a | ||
| // redeem script with listscripts, but the listunspent entry has all the | ||
| // necessary fields already. |
There was a problem hiding this comment.
I believe this is an outdated comment? Don't see a need for redeem script in this method.
server/asset/dcr/dcr.go
Outdated
| func determineTxTree(msgTx *wire.MsgTx) int8 { | ||
| // Try with treasury disabled first. | ||
| txType := stake.DetermineTxType(msgTx, false) | ||
| if txType != stake.TxTypeRegular { | ||
| return wire.TxTreeStake | ||
| } | ||
|
|
||
| // Try with treasury enabled. | ||
| txType = stake.DetermineTxType(msgTx, true) | ||
| if txType != stake.TxTypeRegular { | ||
| return wire.TxTreeStake | ||
| } | ||
|
|
||
| return wire.TxTreeRegular | ||
| } |
There was a problem hiding this comment.
I know this was discussed in matrix at length and this was specifically suggested by @davecgh, but I can't find the discussion now. But even now I don't understand why it's necessary to check treasury disabled first, then enabled.
I thought the only check that might fail was standalone.IsCoinbaseTx, but that's not applicable to stake.DetermineTxType (there is no explicit coinbase check in it, and regular is the default if the stake types do not match). The only possible concern I saw in DetermineTxType was for the new vote tx versions, but the way stake.CheckSSGenVotes is written, it doesn't matter if we just say true.
Hence the following for IsCoinbaseTx in dcrwallet: https://github.com/decred/dcrwallet/blob/703d6cd3e51379683ee38ac24f4ca984b4bfbe0d/internal/compat/compat.go#L20-L28
// IsEitherCoinBaseTx verifies if a transaction is either a coinbase prior to
// the treasury agenda activation or a coinbse after treasury agenda
// activation.
func IsEitherCoinBaseTx(tx *wire.MsgTx) bool {
if standalone.IsCoinBaseTx(tx, false) {
return true
}
if standalone.IsCoinBaseTx(tx, true) {
return true
}
return false
}But everywhere else just saying true for stake.DetermineTxType because the coinbase check is not done directly and saying true for the vote check does not seem to have the potential to give an incorrect result.
Maybe I'm wrong. But we should document the reasoning for all of our treasury handling.
There was a problem hiding this comment.
Looks like you're right. The conversation that led to this change is above, I resolved it previously but I've unresolved it now so it can be easily spotted.
2 things that I see apply from that conversation:
The only things that changed about the old transactions are:
- Regular tree coinbase transactions
- Stake tree votes
So coinbase checks could produce wrong results but as you noted, stake.DetermineTxType does not perform coinbase checks.
It does check votes but again, as you say, the way stake.CheckSSGenVotes is written, it doesn't matter if we just say true.
So I'll defer to @davecgh again to re-confirm if it's perpetually safe to just pass true for isTreasuryEnabled. Will doc this afterwards.
I do see that server/asset/dcr on master currently just passes true:
dcrdex/server/asset/dcr/dcr.go
Line 493 in 93b2fe3
There was a problem hiding this comment.
I've gone with stake.DetermineTxType(msgTx, true) for now until it becomes clearer that we need to check with both false and true.
itswisdomagain
force-pushed
the
update-dcr-deps
branch
2 times, most recently
from
76ad834
to
dc2d673
Compare
itswisdomagain
force-pushed
the
update-dcr-deps
branch
from
dc2d673
to
4183695
Compare
There was a problem hiding this comment.
Looks good. If you resolve and even update to latest module deps we can merge right away.
In a future commit we can use the known treasury activation heights to make DetermineTxType more efficient, but this is correct as is. (https://github.com/decred/dcrdata/blob/a48d34ce35f43ddfead80178fddf58b3e0c417f6/txhelpers/txhelpers.go#L43-L60)
|
Snooping with the dcrwallet rebase, looks like upstream has the "rpc: implement syncstatus JSON-RPC method" commit now. But to summarize what we get from the import of your dcrwallet repo via a So what I'd request if the above is correct is to keep using the As for what dcrwallet executable people need to run when this is merged, it looks like it can be All the dcrd modules seem good for this change as any further gets into the unrelated stdaddr+txscript changes, and @JoeGruffins can update the |
The following modules are updated: - decred.org/dcrwallet => decred.org/dcrwallet/v2@69cae76621d1 - github.com/decred/dcrd/blockchain/stake/v3 => github.com/decred/dcrd/blockchain/stake/v4@5834ce08e290 - github.com/decred/dcrd/dcrutil/v3 => github.com/decred/dcrd/dcrutil/v4@5834ce08e290 - github.com/decred/dcrd/rpc/jsonrpc/types/v2 => github.com/decred/dcrd/rpc/jsonrpc/types/v3@5834ce08e290 - github.com/decred/dcrd/rpcclient/v6 => github.com/decred/dcrd/rpcclient/v7@5834ce08e290 - github.com/decred/dcrd/txscript/v3 => github.com/decred/dcrd/txscript/v4@5834ce08e290 The updated rpcclient ships with signature changes for EstimateSmartFee and GetTxOut methods. The `requiredNodeVersion` is also bumped to match latest dcrd.
itswisdomagain
force-pushed
the
update-dcr-deps
branch
from
4183695
to
e08ba77
Compare
There was a problem hiding this comment.
Just reapproving.
dcrdex:
[INF] DEX: Starting asset backend "dcr"...
[INF] ASSET[dcr]: Connected to dcrd (JSON-RPC API v7.0.0) on SimNet
[DBG] ASSET[dcr]: New block 3ad3669f596c24c119efdd9feee462a3d23fcc871622d371bd59701e5f4b22dc (604)
[TRC] ASSET[dcr]: Notifying 1 dcr asset consumers of new block at height 604
dexc:
[INF] CORE[dcr]: Connected to dcrwallet (JSON-RPC API v8.5.0) proxying dcrd (JSON-RPC API v7.0.0) on SimNet
...
[DBG] CORE[dcr]: Change output size = 36, addr = SsoSsZf84s7Zoy2CVJUDVE2V1o6bJ3A94fy
[DBG] CORE[dcr]: 3 signature cycles to converge on fees for tx 888604607eecf90c646e50eda1a843f28644398df9e62dd4b619e1be115ecdd3: min rate = 11, actual fee rate = 11 (2772 for 252 bytes), change = true
[INF] CORE: notify: |SUCCESS| (feepayment) Fee payment in progress - Waiting for 1 confirmations before trading at 127.0.0.1:17273
[DBG] CORE[dcr]: tip change: 603 (2494af27c7475cabd4fac4abbf2863bbe0f09edfb02f0cf35480b50b4834eace) => 604 (3ad3669f596c24c119efdd9feee462a3d23fcc871622d371bd59701e5f4b22dc)
[TRC] CORE: processing tip change for dcr
[DBG] CORE: Registration fee txn 888604607eecf90c646e50eda1a843f28644398df9e62dd4b619e1be115ecdd3:0 now has 1 confirmations.
...
<order>
...
[INF] CORE: Broadcasted redeem transaction spending 1 contracts for order 9e75cc743674e43bdaf99e660c6e073d6b7a5c912d2ef7bc6ca98a950441bc1f, paying to f458342eada5901e1e0a19d4788894119ff2112cc988a7e78f3f3cd2f2e8a0b0:0 (dcr)
[INF] CORE: Match cac7a050fae31731d9f9ad2c356fdba867577c53b0091c8122089d26f933fa82 complete: buy 1000000000 dcr
[INF] CORE: Notifying DEX 127.0.0.1:17273 of our dcr swap redemption f458342eada5901e1e0a19d4788894119ff2112cc988a7e78f3f3cd2f2e8a0b0:0 for match cac7a050fae31731d9f9ad2c356fdba867577c53b0091c8122089d26f933fa82
The following modules are updated:
The updated
rpcclientships with signature changes toEstimateSmartFeeandGetTxOutmethods.The
requiredNodeVersionis also bumped to match latest dcrd.