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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Feb 15, 2019
Merged

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

merged 1 commit into from Feb 15, 2019

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 11, 2019 •
edited

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.

@maflcko maflcko changed the title [rpc] mining: Report -1 for uninitialized currentblockweight and currentblocktx [rpc] mining: Report -1 for uninitialized currentblockweight, currentblocktx Feb 11, 2019
@benthecarman
Copy link
Contributor

This should probably have release notes

@maflcko
Copy link
Member Author

maflcko commented Feb 11, 2019

I am happy to write them after this is merged

promag
promag reviewed Feb 11, 2019
View reviewed changes
src/rpc/mining.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Member

promag commented Feb 11, 2019

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

@practicalswift
Copy link
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;

@maflcko maflcko changed the title [rpc] mining: Report -1 for uninitialized currentblockweight, currentblocktx [rpc] mining: Report null for uninitialized currentblockweight, currentblocktx Feb 11, 2019
@maflcko
Copy link
Member Author

maflcko commented Feb 11, 2019

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

@promag
Copy link
Member

promag commented Feb 11, 2019

@MarcoFalke could test null values now?

fanquake
fanquake reviewed Feb 11, 2019
View reviewed changes
src/rpc/mining.cpp Outdated Show resolved Hide resolved
@bitcoin bitcoin deleted a comment from maflcko Feb 11, 2019
@luke-jr
Copy link
Member

luke-jr commented Feb 12, 2019

Why not omit the fields entirely?

@gmaxwell
Copy link
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.

promag
promag reviewed Feb 12, 2019
View reviewed changes
Copy link
Member

@promag promag left a comment •
edited

Choose a reason for hiding this comment

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

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

utACK fa3c0da.

doc/release-notes.md Outdated Show resolved Hide resolved
@maflcko maflcko changed the title [rpc] mining: Report null for uninitialized currentblockweight, currentblocktx [rpc] mining: Omit uninitialized currentblockweight, currentblocktx Feb 12, 2019
@maflcko
Copy link
Member Author

maflcko commented Feb 12, 2019

Made it to omit the key-value altogether

@promag
Copy link
Member

promag commented Feb 12, 2019

utACK fa178a6.

@maflcko maflcko added this to the 0.18.0 milestone Feb 12, 2019
@laanwj
Copy link
Member

laanwj commented Feb 12, 2019

Concept ACK

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 15, 2019
Merge bitcoin#15383: [rpc] mining: Omit uninitialized currentblockwei…
eca1273 
…ght, currentblocktx

fa178a6 [rpc] mining: Omit uninitialized currentblockweight, currentblocktx (MarcoFalke)

Pull request description:

  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.

Tree-SHA512: ee94ab203a329e272211b726f4c23edec4b09c650ec363b77fd59ad9264165d73064f78ebb9e11b5c2c543b73c157752410a307655560531c7d5444d203aa0ea
@maflcko maflcko merged commit fa178a6 into bitcoin:master Feb 15, 2019
@maflcko maflcko deleted the Mf1902-rpcMining branch February 15, 2019 14:35
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 4, 2020
@deadalnix
[rpc] mining: Omit uninitialized currentblockweight, currentblocktx
e04b854 
Summary:
This is a backport of Core [[bitcoin/bitcoin#15383 | PR15383]]

Obviously currentblockweight => currentblocksize for us.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5945
@LarryRuane LarryRuane mentioned this pull request Dec 4, 2021
Closed
@str4d str4d mentioned this pull request Dec 6, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake left review comments

@promag promag promag left review comments

Assignees
No one assigned
Labels
Docs Mining RPC/REST/ZMQ
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@maflcko @benthecarman @promag @practicalswift @luke-jr @gmaxwell @laanwj @fanquake