client/asset/btc: fix SPV reorg handling #2225
Conversation
|
Will also probably want to add more unit tests (similar to #2219) here, but existing ones are at least passing. |
|
Thanks.
|
norwnd
force-pushed
the
client-asset-btc-spv-reorg-handling
branch
from
52c8b0d
to
718406b
Compare
chappjc
changed the title
Yep, just squashed, originally decided to do a commit on top in case somebody was already reviewing the 1st commit. |
|
Conditionally tagging this for 0.6, but again I would prefer the unrelated find redemption changes in a separate commit. It needs discussion. |
norwnd
force-pushed
the
client-asset-btc-spv-reorg-handling
branch
from
718406b
to
f97ea5f
Compare
Okay, turned out simple enough to do, moved redemptions search refactor here with that last force-push. |
| Hash: hdr.BlockHash().String(), | ||
| Confirmations: int64(confirms(blockHeight, tip.Height)), | ||
| Height: int64(blockHeight), | ||
| Time: hdr.Timestamp.Unix(), | ||
| }, nil | ||
| Hash: hdr.BlockHash().String(), | ||
| Confirmations: confirmations, | ||
| Height: int64(blockHeight), | ||
| Time: hdr.Timestamp.Unix(), | ||
| PreviousBlockHash: hdr.PrevBlock.String(), |
There was a problem hiding this comment.
Looks like without initialising PreviousBlockHash here we won't be able to to crawl back the chain to find common ancestor with mainchain (instead, we'd nil pointer dereference).
client/asset/btc/spv_wrapper.go
Outdated
| if tip.Height < blockHeight { // if tip is less, may be rolling back, so just mock dcrd/dcrwallet | ||
| confirmations = 0 | ||
| } |
There was a problem hiding this comment.
Even if it's not mainchain? If it's mainchain and tip.Height < blockHeight, confirmations would be set to 0 by the confirms fn.
There was a problem hiding this comment.
I've re-checked with #2219 once again, there is slight discrepancy which should be addressed by 3ded904, thanks for pointing it out!
If it's mainchain and tip.Height < blockHeight, confirmations would be set to 0 by the confirms fn.
I believe when we have tip.Height < blockHeight we can't also have mainchain=true, right ? So this shouldn't be an issue. But to be absolutely sure I've changed this condition a bit to only trigger on mainchain=false.
There was a problem hiding this comment.
I've changed this condition a bit to only trigger on
mainchain=false.
I think @itswisdomagain's point was that this tip.Height < blockHeight check is not required at all because the confirms func handles this, unlike with dcr where we don't have that helper. If blockIsMainchain says false, I don't believe we would overrule that.
In other words, with no knowledge about confirms functionality in this < situation, the code would be:
if mainchain {
if tip.Height < blockHeight { // if tip is less, may be rolling back, so just mock dcrd/dcrwallet
confirmations = 0
} else {
confirmations = int64(confirms(blockHeight, tip.Height))
}
}
return &blockHeader{But confirms handles that already, so it's just
if mainchain {
confirmations = int64(confirms(blockHeight, tip.Height))
}
return &blockHeader{
client/asset/btc/spv_wrapper.go
Outdated
| func (w *spvWallet) getBlockHeader(blockHash *chainhash.Hash) (*blockHeader, error) { | ||
| // getBlockHeader gets the *blockHeader for the specified block hash. It also | ||
| // returns a bool value to indicate whether this block is a part of main chain. | ||
| // For orphaned blocks header.Confirmations is negative (typically -1). |
There was a problem hiding this comment.
The "typically" wording only applies to the RPC code.
client/asset/btc/spv_wrapper.go
Outdated
| if tip.Height < blockHeight { // if tip is less, may be rolling back, so just mock dcrd/dcrwallet | ||
| confirmations = 0 | ||
| } |
There was a problem hiding this comment.
I've changed this condition a bit to only trigger on
mainchain=false.
I think @itswisdomagain's point was that this tip.Height < blockHeight check is not required at all because the confirms func handles this, unlike with dcr where we don't have that helper. If blockIsMainchain says false, I don't believe we would overrule that.
In other words, with no knowledge about confirms functionality in this < situation, the code would be:
if mainchain {
if tip.Height < blockHeight { // if tip is less, may be rolling back, so just mock dcrd/dcrwallet
confirmations = 0
} else {
confirmations = int64(confirms(blockHeight, tip.Height))
}
}
return &blockHeader{But confirms handles that already, so it's just
if mainchain {
confirmations = int64(confirms(blockHeight, tip.Height))
}
return &blockHeader{
client/asset/btc/spv_wrapper.go
Outdated
| func confirms(txHeight, curHeight int32) uint32 { | ||
| switch { | ||
| case txHeight == -1, txHeight > curHeight: | ||
| return 0 | ||
| case curHeight >= txHeight: | ||
| return uint32(curHeight - txHeight + 1) // positive numbers here, no overflow | ||
| default: | ||
| return curHeight - txHeight + 1 | ||
| return 0 // undefined for all other cases |
There was a problem hiding this comment.
One thing we're missing with these changes to confirms is special handling when txHeight is -1. When this helper is used with txHeight, _ := w.cl.GetBlockHeight(blockHash) as in our getBlockHeader method, I don't think we'd ever get -1, however this helper with wtxmgr.TxDetails types like txDetails.Block.Height where -1 is expected in cases and we don't want to change any behavior there e.g. getTxOut.
Note that this confirms helper is lifted directly from dcrwallet: https://github.com/decred/dcrwallet/blob/0aeb02cde83ebb552b802b4eebcc151fe371325c/wallet/wallet.go#L4446-L4456
Let's revert the changes to this function please.
norwnd
force-pushed
the
client-asset-btc-spv-reorg-handling
branch
from
d792bff
to
5e1ceb8
Compare
This PR targeting BTC SPV wallet being able to spot reorgs, as per #2219 (review) (functionally mirroring #2219).
While it is possible to just fix the issue and move on I think some code clean up is warranted here.