[rpc] mining: Omit uninitialized currentblockweight, currentblocktx #15383

MarcoFalke commented at 4:34 PM on February 11, 2019: member

Previously we'd report "0", which could be mistaken for a valid number. E.g. the number of transactions is 0 or the block weight is 0, whatever that means.

MarcoFalke added the label Docs on Feb 11, 2019

MarcoFalke added the label RPC/REST/ZMQ on Feb 11, 2019

MarcoFalke added the label Mining on Feb 11, 2019

MarcoFalke renamed this:
~~[rpc] mining: Report -1 for uninitialized currentblockweight and currentblocktx~~
[rpc] mining: Report -1 for uninitialized currentblockweight, currentblocktx
on Feb 11, 2019

MarcoFalke force-pushed on Feb 11, 2019

benthecarman commented at 7:14 PM on February 11, 2019: contributor

This should probably have release notes

MarcoFalke commented at 7:29 PM on February 11, 2019: member

I am happy to write them after this is merged

MarcoFalke added the label Needs release note on Feb 11, 2019

in src/rpc/mining.cpp:198 in fabf7eb89b outdated

 204 | -            "  \"chain\": \"xxxx\",           (string) current network name as defined in BIP70 (main, test, regtest)\n"
 205 | -            "  \"warnings\": \"...\"          (string) any network and blockchain warnings\n"
 206 | -            "}\n"
 207 | +                    "{\n"
 208 | +                    "  \"blocks\": nnn,             (numeric) The current block\n"
 209 | +                    "  \"currentblockweight\": nnn, (numeric) The block weight of the last assembled block (or -1 if no block was ever assembled)\n"

promag commented at 7:51 PM on February 11, 2019:

Just noting that we could omit these keys instead of -1.

MarcoFalke commented at 7:55 PM on February 11, 2019:

Or set the value to json-null

promag commented at 7:54 PM on February 11, 2019: member

Concept ACK, change looks good too. Could have a test for -1 values?

practicalswift commented at 8:23 PM on February 11, 2019: contributor

Looks good but nLastBlockTx and nLastBlockWeight should be removed from miner.cpp too?

$ git grep -E 'nLastBlockTx|nLastBlockWeight'
src/miner.cpp:uint64_t nLastBlockTx = 0;
src/miner.cpp:uint64_t nLastBlockWeight = 0;

MarcoFalke force-pushed on Feb 11, 2019

MarcoFalke renamed this:
~~[rpc] mining: Report -1 for uninitialized currentblockweight, currentblocktx~~
[rpc] mining: Report null for uninitialized currentblockweight, currentblocktx
on Feb 11, 2019

MarcoFalke removed the label Needs release note on Feb 11, 2019

MarcoFalke commented at 9:15 PM on February 11, 2019: member

Added release notes, removed leftover variables, run clang format, ... Should be ready

MarcoFalke force-pushed on Feb 11, 2019

promag commented at 9:30 PM on February 11, 2019: member

@MarcoFalke could test null values now?

MarcoFalke force-pushed on Feb 11, 2019

in src/rpc/mining.cpp:198 in fa3c0daaf1 outdated

 204 | -            "  \"chain\": \"xxxx\",           (string) current network name as defined in BIP70 (main, test, regtest)\n"
 205 | -            "  \"warnings\": \"...\"          (string) any network and blockchain warnings\n"
 206 | -            "}\n"
 207 | +                    "{\n"
 208 | +                    "  \"blocks\": nnn,             (numeric) The current block\n"
 209 | +                    "  \"currentblockweight\": nnn, (numeric) The block weight of the last assembled block (or 'null' if no block was ever assembled)\n"

fanquake commented at 11:21 PM on February 11, 2019:

(numeric, optional) for both?

MarcoFalke commented at 11:28 PM on February 11, 2019:

They are not optional in a sense that they could be omitted.

The type is either numeric or null-type, but I don't think we document this in other places

promag commented at 11:39 PM on February 11, 2019:

Yap, they are always present, so not optional.

benthecarman deleted a comment on Feb 11, 2019

luke-jr commented at 12:37 AM on February 12, 2019: member

Why not omit the fields entirely?

gmaxwell commented at 1:14 AM on February 12, 2019: contributor

Omitting the fields was also my intuition (just going off the first line of the PR description). It's true that some software might not handle omitted fields well, but the same stuff would probably not handle the special json null either.

in doc/release-notes.md:267 in fa3c0daaf1 outdated

 262 | @@ -263,6 +263,9 @@ in the Low-level Changes section below.
 263 |  
 264 |  - See the [Mining](#mining) section for changes to `getblocktemplate`.
 265 |  
 266 | +- The `getmininginfo` RPC now returns `null` for `currentblockweight` and
 267 | +  `currentblocktx` when a block was never assembled via an RPC on this node.

promag commented at 3:38 PM on February 12, 2019:

nit, just "via RPC"?

promag commented at 3:40 PM on February 12, 2019: member

@luke-jr @gmaxwell it was suggested before #15383 (review). Since it's an edge case, once set it's always present, I tend to prefer null value.

utACK fa3c0da.

[rpc] mining: Omit uninitialized currentblockweight, currentblocktx fa178a6385

MarcoFalke force-pushed on Feb 12, 2019

MarcoFalke renamed this:
~~[rpc] mining: Report null for uninitialized currentblockweight, currentblocktx~~
[rpc] mining: Omit uninitialized currentblockweight, currentblocktx
on Feb 12, 2019

MarcoFalke commented at 4:37 PM on February 12, 2019: member

Made it to omit the key-value altogether

promag commented at 4:43 PM on February 12, 2019: member

utACK fa178a6.

MarcoFalke added this to the milestone 0.18.0 on Feb 12, 2019

laanwj commented at 4:55 PM on February 12, 2019: member

Concept ACK

MarcoFalke referenced this in commit eca1273c35 on Feb 15, 2019

MarcoFalke merged this on Feb 15, 2019

MarcoFalke closed this on Feb 15, 2019

MarcoFalke deleted the branch on Feb 15, 2019

deadalnix referenced this in commit e04b854d9b on May 4, 2020

MarcoFalke locked this on Dec 16, 2021