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

fix verbocity in getblock command #1112

Closed
wants to merge 2 commits into from
Closed

fix verbocity in getblock command #1112

wants to merge 2 commits into from

Conversation

asidorochev
Copy link

Original "getblock" command accept 2 arguments:

  1. blockhash
  2. verbosity (numeric, optional, default=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data

Here is a fix to use getblock command in the same way.

@hh532
Copy link

hh532 commented Feb 28, 2018

Hi asidorochev,

I am running into the same problem as here which requires this fix. When will this be merged?

Thanks!

@asidorochev
Copy link
Author

@henrykhadass Unfortunately, it does not depend on me.

@hh532
Copy link

hh532 commented Feb 28, 2018

@asidorochev I see, you need one of the core developers to review etc?

I suppose in the meantime the following is a work around, which does the same:

blockHeight := 110000
blockHash, err := client.GetBlockHash(int64(blockHeight))
if err != nil {
	log.Fatal(err)
}

blockVerboseTx, err := client.GetBlockVerbose(blockHash)
if err != nil {
	log.Fatal(err)
}

for _, txid := range blockVerboseTx.Tx {
	txidHash, err := chainhash.NewHashFromStr(txid)
	if err != nil {
		log.Fatal(err)
	}

	rawTx, err := client.GetRawTransactionVerbose(txidHash)
	if err != nil {
		log.Fatal(err)
	}
.......

Would be great to get this merged in!

Copy link

@kerlw kerlw left a comment

Choose a reason for hiding this comment

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

This should merged into master as soon as we can!

@cabezi
Copy link

cabezi commented Apr 9, 2018

@asidorochev Clearly there has been a marshal mistake in github.com/btcsuite/btcd/rpcclient/chain.go

type FutureGetBlockVerboseResult chan *response
// Receive waits for the response promised by the future and returns the data
// structure from the server with information about the requested block.


func (r FutureGetBlockVerboseResult) Receive() (*btcjson.GetBlockVerboseResult, error) {
   res, err := receiveFuture(r)
   if err != nil {
   	return nil, err
   }

   // Unmarshal the raw result into a BlockResult.
   var blockResult btcjson.GetBlockVerboseResult
   err = json.Unmarshal(res, &blockResult)
   if err != nil {
   	return nil, err
   }
   return &blockResult, nil
}

error:
panic: json: cannot unmarshal object into Go struct field GetBlockVerboseResult.tx of type string [recovered] panic: json: cannot unmarshal object into Go struct field GetBlockVerboseResult.tx of type string

@Ronmi
Copy link

Ronmi commented May 9, 2018

The response from bitcoind reused field "tx" to store two different types of data. It might be better to separate the response type of "GetBlockVerboseTx" from "GetBlockVerbose".

Here's an example diff on top of this PR, which is used in my own project:

diff --git a/btcjson/chainsvrresults.go b/btcjson/chainsvrresults.go
index 33d54d3..b93929f 100644
--- a/btcjson/chainsvrresults.go
+++ b/btcjson/chainsvrresults.go
@@ -28,23 +28,15 @@ type GetBlockHeaderVerboseResult struct {
 // verbose flag is set.  When the verbose flag is not set, getblock returns a
 // hex-encoded string.
 type GetBlockVerboseResult struct {
-	Hash          string        `json:"hash"`
-	Confirmations uint64        `json:"confirmations"`
-	StrippedSize  int32         `json:"strippedsize"`
-	Size          int32         `json:"size"`
-	Weight        int32         `json:"weight"`
-	Height        int64         `json:"height"`
-	Version       int32         `json:"version"`
-	VersionHex    string        `json:"versionHex"`
-	MerkleRoot    string        `json:"merkleroot"`
-	Tx            []string      `json:"tx,omitempty"`
-	RawTx         []TxRawResult `json:"rawtx,omitempty"`
-	Time          int64         `json:"time"`
-	Nonce         uint32        `json:"nonce"`
-	Bits          string        `json:"bits"`
-	Difficulty    float64       `json:"difficulty"`
-	PreviousHash  string        `json:"previousblockhash"`
-	NextHash      string        `json:"nextblockhash,omitempty"`
+	GetBlockHeaderVerboseResult
+	Tx []string `json:"tx,omitempty"`
+}
+
+// GetBlockVerboseTxResult represnets the data from the getblock command when the
+// verbose flag is set to 2.
+type GetBlockVerboseTxResult struct {
+	GetBlockHeaderVerboseResult
+	Tx []TxRawResult `json:"tx"`
 }
 
 // CreateMultiSigResult models the data returned from the createmultisig
diff --git a/rpcclient/chain.go b/rpcclient/chain.go
index e8a9ba9..995d6f3 100644
--- a/rpcclient/chain.go
+++ b/rpcclient/chain.go
@@ -154,12 +154,33 @@ func (c *Client) GetBlockVerbose(blockHash *chainhash.Hash) (*btcjson.GetBlockVe
 	return c.GetBlockVerboseAsync(blockHash).Receive()
 }
 
+// FutureGetBlockVerboseTxResult is a future promise to deliver the result of a
+// GetBlockVerboseTxAsync RPC invocation (or an applicable error).
+type FutureGetBlockVerboseTxResult chan *response
+
+// Receive waits for the response promised by the future and returns the data
+// structure from the server with information about the requested block.
+func (r FutureGetBlockVerboseTxResult) Receive() (*btcjson.GetBlockVerboseTxResult, error) {
+	res, err := receiveFuture(r)
+	if err != nil {
+		return nil, err
+	}
+
+	// Unmarshal the raw result into a BlockResult.
+	var blockResult btcjson.GetBlockVerboseTxResult
+	err = json.Unmarshal(res, &blockResult)
+	if err != nil {
+		return nil, err
+	}
+	return &blockResult, nil
+}
+
 // GetBlockVerboseTxAsync returns an instance of a type that can be used to get
 // the result of the RPC at some future time by invoking the Receive function on
 // the returned instance.
 //
 // See GetBlockVerboseTx or the blocking version and more details.
-func (c *Client) GetBlockVerboseTxAsync(blockHash *chainhash.Hash) FutureGetBlockVerboseResult {
+func (c *Client) GetBlockVerboseTxAsync(blockHash *chainhash.Hash) FutureGetBlockVerboseTxResult {
 	hash := ""
 	if blockHash != nil {
 		hash = blockHash.String()
@@ -175,7 +196,7 @@ func (c *Client) GetBlockVerboseTxAsync(blockHash *chainhash.Hash) FutureGetBloc
 //
 // See GetBlockVerbose if only transaction hashes are preferred.
 // See GetBlock to retrieve a raw block instead.
-func (c *Client) GetBlockVerboseTx(blockHash *chainhash.Hash) (*btcjson.GetBlockVerboseResult, error) {
+func (c *Client) GetBlockVerboseTx(blockHash *chainhash.Hash) (*btcjson.GetBlockVerboseTxResult, error) {
 	return c.GetBlockVerboseTxAsync(blockHash).Receive()
 }

Provide to @asidorochev as a suggestion, hope this PR can be merged soon.

@reinerRubin
Copy link

I have locally applied @Ronmi Ronmi diff to this PR and it works. I am ready to make more changes/tests/whatever, but I do not know what to do exactly. Can somebody point?

@zhiyuan2007
Copy link

just find , i also make a pr

@reinerRubin
Copy link

@cabezi, @Ronmi, @asidorochev.
Can you check my pr? I fixed the unmarshal fail (thx @Ronmi) and made corresponded changes. We heavily use the btcsuit sub-projects in our projects, so it would be terrible to keep a fork.

@lzl124631x
Copy link

+1. Please merge this?

@justinmoon
Copy link

I just ran into this incompatibility problem today. Would be nice for this RPC call to have exactly the same semantics as bitcoind.

Copy link

@justinmoon justinmoon left a comment

Choose a reason for hiding this comment

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

Comments on style and documentation

@@ -1054,13 +1054,13 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
}
}

// When the verbose flag isn't set, simply return the serialized block
// When the verbosity value setted to 0, simply return the serialized block

Choose a reason for hiding this comment

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

grammar nitpick: "When the verbose flag is set to 0"

@@ -1109,7 +1109,8 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
NextHash: nextHashString,
}

if c.VerboseTx == nil || !*c.VerboseTx {
// When the verbosity value isn't set, setted to 1 or not equal 0 or 2
if c.Verbosity == nil || *c.Verbosity == 1 || (*c.Verbosity != 0 && *c.Verbosity != 2) {
Copy link

@justinmoon justinmoon Jun 24, 2019

Choose a reason for hiding this comment

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

isn't this line equivalent to if *c.Verbosity != 0 && *c.Verbosity != 2?

"getblock-verbosetx": "Specifies that each transaction is returned as a JSON object and only applies if the verbose flag is true (btcd extension)",
"getblock--condition0": "verbose=false",
"getblock--condition1": "verbose=true",
"getblock-verbosity": "Specifies the block format returns",
Copy link

@justinmoon justinmoon Jun 24, 2019

Choose a reason for hiding this comment

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

The rest of the help uses command-verbose and verbose=n patterns, not a command-verbosity and verbosity=n. For example,

btcd/rpcserverhelp.go

Lines 463 to 466 in ab8fa7b

"getrawmempool-verbose": "Returns JSON object when true or an array of transaction hashes when false",
"getrawmempool--condition0": "verbose=false",
"getrawmempool--condition1": "verbose=true",
"getrawmempool--result0": "Array of transaction hashes",

return &GetBlockCmd{
Hash: hash,
Verbose: verbose,
VerboseTx: verboseTx,
Verbosity: verbosity,

Choose a reason for hiding this comment

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

The rest of this file uses Verbose as a variable name, not Verbosity. For example,

Verbose *bool `jsonrpcdefault:"false"`

@galileo-pkm
Copy link

I just ran into this incompatibility problem today. Would be nice for this RPC call to have exactly the same semantics as bitcoind.

I would not hold by breath, it has been sitting in limbo for almost a year, use the PR from
@reinerRubin it works without issues.

@tiger5226
Copy link

Yeah this sucks...no one can approve a review? If it's just review changes that are needed I can add those.

@jakesylvestre
Copy link
Collaborator

@jcvernaleo (as per #1530)

  • High priority
  • Bug

This also fixes the issue preventing @jalavosus PR #1529 from being merged

@jcvernaleo
Copy link
Member

jcvernaleo commented Mar 19, 2020

This needs to be rebased, have the conflict fixed, and if @justinmoon's comments could be address. If you can do those 3 things I can do final review and merge.

Realized this comment should have been a change request, not just a comment.

Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

This needs to be rebased, have the conflict fixed, and if @justinmoon's comments could be address. If you can do those 3 things I can do final review and merge.

@tiger5226
Copy link

tiger5226 commented Mar 21, 2020

This can be closed. The change have been made in another PR and merged into master.

c4f3999

@jcvernaleo jcvernaleo closed this Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.