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

rpc: implement getcfilterv2 and getblock JSON-RPC methods #1978

Merged
merged 6 commits into from May 28, 2021

Conversation

itswisdomagain
Copy link
Member

Both methods closely resemble dcrd's implementations, with the exception of
the data returned by the getcfilterv2 method:

  • the filter is returned as json:"filter" instead of json:"data".
  • the data returned includes the key required to query the returned filter for
    matches against committed scripts.

Size: int32(blk.Header.Size),
Bits: strconv.FormatInt(int64(blockHeader.Bits), 16),
SBits: sbitsFloat,
Difficulty: difficultyRatio(blockHeader.Bits, w.ChainParams()),
Copy link
Member

Choose a reason for hiding this comment

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

It looks like dcrwallet's difficultyRatio differs from dcrd's. dcrd does this additional intermediate step.

outString := difficulty.FloatString(8)
diff, err := strconv.ParseFloat(outString, 64)

And indeed, when I compare the getblock output, I see "difficulty": 1.66277624 for dcrd and "difficulty": 1.6627762413416893 for dcrwallet.

Maybe it doesn't really matter. I don't know.

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... I noticed that. I did not compare the outputs but the code difference looked like a simple case of output formatting. dcrd rounds to 8 decimal places using difficulty.FloatString(8) before converting the resulting value to a float64; while dcrwallet converts directly to float64 using difficulty.Float64().

I don't suppose there's an implication to not rounding first, but let me ping @davecgh to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for giving this a look!

Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't really matter since nothing should be using that value in any calculations as it's really there to provide a more human-readable value.

That said, the result should not be more than 8 decimal places long for at least a couple of reasons:

  1. It represents a ratio of a process that involves 256 bits and log_2(256) = 8
  2. It really should match what dcrd does anyway so you don't get different results

itswisdomagain added a commit to itswisdomagain/dcrwallet that referenced this pull request Mar 11, 2021
itswisdomagain added a commit to itswisdomagain/dcrwallet that referenced this pull request Mar 31, 2021
@chappjc
Copy link
Member

chappjc commented May 3, 2021

Aside from the conflicts on this PR and #1903, how are they looking?

@itswisdomagain
Copy link
Member Author

Aside from the conflicts on this PR and #1903, how are they looking?

All good. Ready for review and merging.

Copy link
Member

@jrick jrick left a comment

Choose a reason for hiding this comment

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

seems to work but have some nits and q's.

Comment on lines 1259 to 1263
blkBytes, err := blk.Bytes()
if err != nil {
return nil, rpcErrorf(dcrjson.ErrRPCInternal.Code, "Could not serialize block: %v", err)
}
return hex.EncodeToString(blkBytes), nil
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 avoid calling Bytes. this should instead use a hex.Encoder. there are some other examples of this technique in this file but this is the gist:

        b := new(strings.Builder)
        b.Grow(2 * blk.SerializeSize())
        err = blk.Serialize(hex.NewEncoder(b))
        return b.String(), nil

}
nextHashString = nextBlockInfo.Hash.String()
}
confirmations = 1 + int64(bestHeight) - int64(blockHeader.Height)
Copy link
Member

Choose a reason for hiding this comment

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

use the confirms function to calculate this.


// When the verbose flag isn't set, simply return the
// network-serialized block as a hex-encoded string.
if cmd.Verbose != nil && !*cmd.Verbose {
Copy link
Member

Choose a reason for hiding this comment

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

if cmd.Verbose == nil || !*cmd.Verbose {

transactions := blk.Transactions
txNames := make([]string, len(transactions))
for i, tx := range transactions {
txNames[i] = tx.CachedTxHash().String()
Copy link
Member

Choose a reason for hiding this comment

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

Use TxHash, not CachedTxHash (here and below).

}

txReply := &dcrdtypes.TxRawResult{
Hex: hex.EncodeToString(mtxBytes),
Copy link
Member

Choose a reason for hiding this comment

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

use the hex encoder to write to a strings.Builder like above.

Comment on lines +1565 to +1579
// The minimum difficulty is the max possible proof-of-work limit bits
// converted back to a number. Note this is not the same as the proof
// of work limit directly because the block difficulty is encoded in a
// block with the compact form which loses precision.
max := blockchain.CompactToBig(params.PowLimitBits)
target := blockchain.CompactToBig(bits)
ratio, _ := new(big.Rat).SetFrac(max, target).Float64()
return ratio

difficulty := new(big.Rat).SetFrac(max, target)
outString := difficulty.FloatString(8)
diff, err := strconv.ParseFloat(outString, 64)
if err != nil {
log.Errorf("Cannot get difficulty: %v", err)
return 0
}
return diff
Copy link
Member

Choose a reason for hiding this comment

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

not clear why you are changing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Primarily to mach dcrd's output; this change here mirrors dcrd's difficultyRatio. There was a conversation about it here.

@jrick jrick merged commit fae3a09 into decred:master May 28, 2021
@itswisdomagain itswisdomagain deleted the cfilters-rawblocks branch May 29, 2021 07:10
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

5 participants