Skip to content
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.

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

client/asset/dcr: refactor findRedemptionsInBlockRange to use cfilters #954

Merged

Conversation

itswisdomagain
Copy link
Member

@itswisdomagain itswisdomagain commented Jan 30, 2021

Also

  • modify TestFindRedemption to test finding redeem txs in mempool
  • fix re-org depth calculation in {dcr,btc}.checkNewBlocks.

@chappjc chappjc added this to the 0.3 milestone Apr 7, 2021
@itswisdomagain itswisdomagain force-pushed the dcr-findredemption-with-cfilters branch 2 times, most recently from 97e9ac7 to 7086e25 Compare July 27, 2021 19:25
@itswisdomagain itswisdomagain marked this pull request as ready for review July 27, 2021 19:48
@chappjc chappjc self-requested a review July 27, 2021 21:07
@chappjc chappjc added the SPV label Aug 2, 2021
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes, esp. fixing checkForNewBlocks. Mostly some nits, but also requesting to change to getblockheader in checkForNewBlocks.

BTW, TestMakerGhostingAfterTakerRedeem is passing as expected.

client/asset/dcr/dcr.go Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
}

// Pull the block info to confirm if any of its inputs spends a contract of interest.
blk, err := dcr.node.GetBlockVerbose(dcr.ctx, blockHash, true)
Copy link
Member

@chappjc chappjc Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but just a note: we don't really need getblock with either verbose=true or verboseTx=true since with a block's bytes we could deserialize and work on the MsgTxs. No changes requested though. It's going to be a headache to change, esp. on account of the helper methods needing TxRawResults. Besides this is not a hot path in dex client code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add a todo to that effect.

@itswisdomagain itswisdomagain force-pushed the dcr-findredemption-with-cfilters branch from 7086e25 to 8f7a617 Compare August 3, 2021 23:58
@itswisdomagain
Copy link
Member Author

dcr, btc simnet tests and TestMakerGhostingAfterTakerRedeem passing

@@ -2048,6 +2084,33 @@ func (dcr *ExchangeWallet) fatalFindRedemptionsError(err error, contractOutpoint
dcr.findRedemptionMtx.Unlock()
}

// blockMaybeContainsScripts uses the cfilters of the specified block to
// determine if the block likely includes any of the passed scripts.
func (dcr *ExchangeWallet) blockMaybeContainsScripts(blockHash string, scripts [][]byte) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remember to refactor this method in terms of the blockFilter struct and the getBlockFilterV2 methods slated to be added in #788

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this will likely go in favor of that. Wanted a simple version in first. Or rather, this will use the new methods and struct.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and test well. Just the mutex change in the test.

I'll leave this up for a day for others to check out. I'm 50% through #788 at the moment, and it's looking pretty good too.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and works. We should fix that reorg script...

@itswisdomagain
Copy link
Member Author

We should fix that reorg script...

Yeah. I want to revisit that today.

This changes the client's DCR backend to use v2 cfilters to locate
counterparty contract redemptions in blocks instead of always
pulling blocks to check.

This also adds a test for the mempool redemption search.
It is necessary to crawl backwards from the previous main chain
tip to a main chain block (confs>0), the ancestor, to determine
reorg depth.

This logic is used to trigger findRedemptionsInBlockRange.
@chappjc chappjc force-pushed the dcr-findredemption-with-cfilters branch from 3333748 to 53697ed Compare August 4, 2021 15:30
@chappjc chappjc merged commit cfa95eb into decred:master Aug 4, 2021
@itswisdomagain itswisdomagain deleted the dcr-findredemption-with-cfilters branch August 6, 2021 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants