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

node/http: add next interval #175

Closed
wants to merge 3 commits into from
Closed

node/http: add next interval #175

wants to merge 3 commits into from

Conversation

tynes
Copy link
Contributor

@tynes tynes commented May 14, 2019 •
edited
Loading

Adds 2 new properties to Node GET /

  • nextTreeRootHeight - blockheight at which the tree will be committed
  • blocksUntilNextTreeRoot - number of blocks until the next time the tree is committed

This is useful for applications building on top of hsd that need to know the next time the tree will be committed. It also includes tests for off by one errors.

@codecov-io
Copy link

codecov-io commented May 14, 2019 •
edited
Loading

Codecov Report

Merging #175 into master will decrease coverage by 8.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   52.99%   44.91%   -8.08%     
==========================================
  Files         129      126       -3     
  Lines       35773    35080     -693     
  Branches     6032     5952      -80     
==========================================
- Hits        18957    15757    -3200     
- Misses      16816    19323    +2507
Impacted Files Coverage Δ
lib/node/http.js 41.16% <100%> (-8.14%) ⬇️
lib/dns/compress.js 3.32% <0%> (-46.87%) ⬇️
lib/mempool/fees.js 30.41% <0%> (-35.77%) ⬇️
lib/wallet/wallet.js 31.72% <0%> (-29.69%) ⬇️
lib/mempool/contractstate.js 50.24% <0%> (-24.88%) ⬇️
lib/net/hostlist.js 19.69% <0%> (-22.99%) ⬇️
lib/wallet/txdb.js 56.98% <0%> (-20.9%) ⬇️
lib/wallet/http.js 45.97% <0%> (-19.07%) ⬇️
lib/dns/resource.js 1.97% <0%> (-17.84%) ⬇️
lib/net/peer.js 3.65% <0%> (-16.1%) ⬇️
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc1ef7a...d660fa0. Read the comment docs.

@kilpatty
Copy link
Contributor

Good catch, this is actually super helpful for HNScan!

@tynes tynes added api labels May 14, 2019
@tynes
Copy link
Contributor Author

tynes commented May 14, 2019

I wasn't sure what would be more useful, blocksUntilNextInterval or nextInterval.
Ended up going with nextInterval because its shorted and you can calculate blocksUntilNextInterval locally

@kilpatty
Copy link
Contributor

kilpatty commented May 14, 2019 •
edited
Loading

No test for OBOE??

Also thoughts about adding this into get name resource? I ran into some issues locally where seeing when a record that I added to a name would update is not explicitly clear. I wonder if we should think about adding this into the "info" part of a name when we pull a name.

tynes
tynes commented May 16, 2019
View reviewed changes
lib/node/http.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented May 23, 2019

@kilpatty

No test for OBOE??

Added test coverage for an off by one error

Also thoughts about adding this into get name resource?

We could definitely add this here, I think ns.getJSON is a good place for it. Out of scope for this PR though

boymanjor
boymanjor reviewed May 24, 2019
View reviewed changes
test/util/common.js Outdated Show resolved Hide resolved
boymanjor
boymanjor reviewed May 24, 2019
View reviewed changes
test/util/common.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented May 25, 2019

@boymanjor Resolved issues, pushed up new code

@tynes tynes force-pushed the getinfo-nextinterval branch 2 times, most recently from 8386649 to 963130e Compare May 25, 2019 00:20
Mark Tyneway added 3 commits June 22, 2019 11:31
node/http: add next interval
9794590 
Add `chain.nextInterval` to the response of
`GET /` on the node HTTP server. The value
is the block height of the next time that the
`treeRoot` is committed to disk along with
being included in the block headers.
node/http: return interval height and blocks until
bfe2b80 
To prevent bugs on the client side, return
both the number of blocks until the next
tree committment and return the height of
the next tree committment. Application builders
may need either value and while it is possible
to compute the other with one, its better
to just return both.
test: add node-http-test
d660fa0
@pinheadmz
Copy link
Member

@tynes I think this is a useful datum to return here, do you still wanna finish? I could take it over if you like.

Thoughts:

  • Probably don't need the privatekey/wallet stuff in the test, lets remove anything we don't really need
  • I think we could just add one value: nextTreeUpdateHeight with the height of the next update.

@nodech nodech added needs rebase modifier and removed api labels Nov 18, 2021
@nodech nodech added node-http part of the codebase tests part of the codebase quick review difficulty - easy waiting for the author modifier labels Nov 18, 2021
@nodech nodech added the breaking-minor Backwards compatible - Release version label Dec 10, 2021
@nodech nodech mentioned this pull request Dec 13, 2021
Open
@nodech nodech closed this Dec 13, 2021
@nodech nodech mentioned this pull request Oct 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boymanjor boymanjor boymanjor left review comments

Assignees

@boymanjor boymanjor

Labels
breaking-minor Backwards compatible - Release version needs rebase modifier node-http part of the codebase quick review difficulty - easy tests part of the codebase waiting for the author modifier
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tynes @codecov-io @kilpatty @pinheadmz @boymanjor @nodech