Adding confirmations property to block and tx getJSON

[sangaman]

Adds a confirmations property to the JSON returned by cli tx [hash] and cli block [hash/height] as
well as the REST /tx/:hash and /block/:block calls by subtracting TX/block height from chain height.

I'm new to this codebase but I tried to implement this as cleanly and sensibly as I could. It passed all mocha tests as well as some manual testing with regtest.

This addresses issue #320.

[Sexual]

Why would confirmations be added to the block endpoint?

[sangaman]

@Sexual, I had that thought too, but it appears to be consistent with bitcoind's getblock call. I haven't gotten a chance to try the call against a bitcoind instance myself yet, but according to https://chainquery.com/bitcoin-api/getblock it appears that it returns confirmations as shown below:

Result (for verbose = true):
{
"hash" : "hash", (string) the block hash (same as provided)
"confirmations" : n, (numeric) The number of confirmations, or -1 if the block is not on the main chain
"size" : n, (numeric) The block size
"strippedsize" : n, (numeric) The block size excluding witness data
"weight" : n (numeric) The block weight as defined in BIP 141
"height" : n, (numeric) The block height or index
"version" : n, (numeric) The block version
"versionHex" : "00000000", (string) The block version formatted in hexadecimal
"merkleroot" : "xxxx", (string) The merkle root
"tx" : [ (array of string) The transaction ids
"transactionid" (string) The transaction id
,...
],
"time" : ttt, (numeric) The block time in seconds since epoch (Jan 1 1970 GMT)
"mediantime" : ttt, (numeric) The median block time in seconds since epoch (Jan 1 1970 GMT)
"nonce" : n, (numeric) The nonce
"bits" : "1d00ffff", (string) The bits
"difficulty" : x.xxx, (numeric) The difficulty
"chainwork" : "xxxx", (string) Expected number of hashes required to produce the chain up to this block (in hex)
"previousblockhash" : "hash", (string) The hash of the previous block
"nextblockhash" : "hash" (string) The hash of the next block
}

I figure it's a quick an easy way to verify a block is part of the main chain and to see how deeply embedded it is.

Rereading the bold part above does make me think that maybe I should add in logic to return -1 for confirmations if the block is not part of the main chain to match the behavior described above, I think currently it would just return null. I can work on that tonight.

[sangaman]

The getblock behavior for returning confirmations is also described here https://bitcoin.org/en/developer-reference#getblock and confirms that its value should be -1 if the block is not in the main chain, so this shouldn't be merged until I implement that.

[sangaman]

Looks like it would make sense to try and implement the same logic for unconfirmed/conflicted (which I assume means it's a doublespend attempt) transactions as described here as well https://bitcoin.org/en/developer-reference#gettransaction:

The number of confirmations the transaction has received. Will be 0 for unconfirmed and -1 for conflicted

[sangaman]

I refined this a bit. I previously was failing to add 1 to confirmations since a tx or block at the tip of the main chain starts with 1 confirmation and tx-or-block-height minus chain-height alone equals zero in that case. I added logic to check if a block is orphaned and return -1 for confirmations in that case, however I couldn't figure out how to test this using regtest and I couldn't find any orphans with testnet to test with since I only recently synced it.

Unconfirmed transactions will have 0 for confirmations, however I couldn't figure out how to check for a transaction that's an attempted doublespend. It looks like mempool.js will refuse to add a transaction that spends an input that's already being used by a different transaction in the mempool, which means it knowledge of the attempted doublespend is simply discarded (aside from a log entry). If that assessment is correct, maybe tracking doublespend attempts would be a nice feature to have?

One last consideration is that this change would put the client out of sync with the examples at http://bcoin.io/api-docs/. It looks like that isn't part of this repository though.

[bucko13]

Thanks for adding this support @sangaman and keeping on top of the PR. For the docs, all of that info is managed in the bcoin.io github pages repo. That is also all open though as well, so feel free to submit an issue with the changes to the API that would be necessary once this gets approved.

[sangaman]

Thanks for linking me to that repo, I tried looking for it earlier but must have missed it. I created a pull request there to sync with the changes here.

[chjj]

Looks good, but just a warning that we're running dangerously close to 4 parameters for that function. I think it's a good rule to keep it at 3 or under, and never over 4. Could be that the chain height is the last thing we can justify adding. :)