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

Add getchaintxstats RPC #9733

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@sipa
Member

sipa commented Feb 10, 2017

Not intended for 0.14, but may be used to compute the new chainTxData constants in releases.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 10, 2017

Member

Alternative: don't allow specifying the window size, and include the output in getblockchaininfo.

Member

sipa commented Feb 10, 2017

Alternative: don't allow specifying the window size, and include the output in getblockchaininfo.

Show outdated Hide outdated src/rpc/blockchain.cpp Outdated
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 10, 2017

Contributor

I prefer having it separate from getblockchaininfo because it brings more flexibility - ability to query past data etc. On the other hand, adding current txrate and txcount to getblockchaininfo: why not :-)

Contributor

paveljanik commented Feb 10, 2017

I prefer having it separate from getblockchaininfo because it brings more flexibility - ability to query past data etc. On the other hand, adding current txrate and txcount to getblockchaininfo: why not :-)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 10, 2017

Member

Concept ACK. Though I wonder if this shouldn't be an external script, if it's only used for computing some statistics before releases.

On the other hand, adding current txrate and txcount to getblockchaininfo: why not :-)

Well we shouldn't add anything that is non-trivial to compute to getblockchaininfo. I think restricting that call to return state information that is up-to-date and used by the client anyway makes sense. Otherwise it becomes slower and slower as more things are added, like what happened to getinfo.
So a separate call is fine with me.

Member

laanwj commented Feb 10, 2017

Concept ACK. Though I wonder if this shouldn't be an external script, if it's only used for computing some statistics before releases.

On the other hand, adding current txrate and txcount to getblockchaininfo: why not :-)

Well we shouldn't add anything that is non-trivial to compute to getblockchaininfo. I think restricting that call to return state information that is up-to-date and used by the client anyway makes sense. Otherwise it becomes slower and slower as more things are added, like what happened to getinfo.
So a separate call is fine with me.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 5, 2017

Member

Though I wonder if this shouldn't be an external script, if it's only used for computing some statistics before releases.

I don't think it's unreasonable to have a built-in function to compute the transaction rate.

Member

sipa commented Apr 5, 2017

Though I wonder if this shouldn't be an external script, if it's only used for computing some statistics before releases.

I don't think it's unreasonable to have a built-in function to compute the transaction rate.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 5, 2017

Member

I don't think it's unreasonable to have a built-in function to compute the transaction rate.

What's the use case for this, other than computing the chainTxData constants?

Member

jnewbery commented Apr 5, 2017

I don't think it's unreasonable to have a built-in function to compute the transaction rate.

What's the use case for this, other than computing the chainTxData constants?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 6, 2017

Member

What's the use case for this, other than computing the chainTxData constants?

Not really a use case, except that's it's generally useful information that someone may be interested in.

Member

sipa commented Apr 6, 2017

What's the use case for this, other than computing the chainTxData constants?

Not really a use case, except that's it's generally useful information that someone may be interested in.

@TheBlueMatt

utACK +/- the two nits

Show outdated Hide outdated src/rpc/blockchain.cpp Outdated
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 24, 2017

Member

utACK 53d4292
Although for plotting things an interface like initialBlockHeight, endBlockHeight feel more natural than the distance and the hash of the block. It seems there's no functionality loss since the block hash cannot be used to select a chain that is not part of the active chain anyway.

Member

jtimon commented Apr 24, 2017

utACK 53d4292
Although for plotting things an interface like initialBlockHeight, endBlockHeight feel more natural than the distance and the hash of the block. It seems there's no functionality loss since the block hash cannot be used to select a chain that is not part of the active chain anyway.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 2, 2017

Member

Can be merged if the small nits by @TheBlueMatt and @paveljanik are fixed

Member

laanwj commented May 2, 2017

Can be merged if the small nits by @TheBlueMatt and @paveljanik are fixed

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 3, 2017

Member

@laanwj Done.

Member

sipa commented May 3, 2017

@laanwj Done.

UniValue getchaintxstats(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() > 2)
throw runtime_error(

This comment has been minimized.

@laanwj

laanwj May 3, 2017

Member

looks like travis complains now: the using namespace was removed, so this should be std::runtime_error, will fix this up before merging

@laanwj

laanwj May 3, 2017

Member

looks like travis complains now: the using namespace was removed, so this should be std::runtime_error, will fix this up before merging

laanwj added a commit that referenced this pull request May 3, 2017

Merge #9733: Add getchaintxstats RPC
bd1f138 Add getchaintxstats RPC (Pieter Wuille)

Tree-SHA512: 270785b25e7e2faad4528b5ef591d9dc6266f15236563e3f02dac1f2d9ce3732c4d44903fcccf38549f7921f29d1a93cb0a118b7453ccc5afd79739b51e68f46
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 3, 2017

Member

Merged via d4732f3

Member

laanwj commented May 3, 2017

Merged via d4732f3

@laanwj laanwj closed this May 3, 2017

@laanwj laanwj referenced this pull request May 3, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete

@jnewbery jnewbery referenced this pull request Jul 13, 2017

Closed

Add histunspent RPC #10804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment