Skip to content

StateProofs: Add timeout to stateproof api#4515

Merged
winder merged 7 commits into
algorand:masterfrom
algonathan:add-timeout-to-stateproof-api
Sep 8, 2022
Merged

StateProofs: Add timeout to stateproof api#4515
winder merged 7 commits into
algorand:masterfrom
algonathan:add-timeout-to-stateproof-api

Conversation

@algonathan
Copy link
Copy Markdown
Contributor

@algonathan algonathan commented Sep 8, 2022

Summary

/v2/stateproofs/{round} endpoint on algod is responsible for finding a state proof that covers the given round. There are some edge cases in which this search might take a long time. This PR adds timeout to the search.

Test Plan

Add some uint tests + manual test

@algonathan algonathan requested a review from id-ms September 8, 2022 11:01
@algonathan algonathan self-assigned this Sep 8, 2022
Comment thread daemon/algod/api/server/v2/handlers.go
Comment thread daemon/algod/api/server/v2/handlers.go
@id-ms id-ms added the Bug-Fix label Sep 8, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 8, 2022

Codecov Report

Merging #4515 (97283cb) into master (00b37f2) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4515      +/-   ##
==========================================
- Coverage   55.28%   55.25%   -0.03%     
==========================================
  Files         398      398              
  Lines       50336    50353      +17     
==========================================
- Hits        27829    27824       -5     
- Misses      20178    20199      +21     
- Partials     2329     2330       +1     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
daemon/algod/api/server/v2/utils.go 13.19% <0.00%> (-0.10%) ⬇️
ledger/blockqueue.go 84.48% <0.00%> (-4.03%) ⬇️
agreement/cryptoVerifier.go 67.60% <0.00%> (-2.12%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
ledger/acctupdates.go 69.59% <0.00%> (-0.60%) ⬇️
catchup/service.go 68.88% <0.00%> (-0.50%) ⬇️
network/wsNetwork.go 64.82% <0.00%> (+0.19%) ⬆️
network/wsPeer.go 68.49% <0.00%> (+3.01%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

id-ms
id-ms previously approved these changes Sep 8, 2022
Copy link
Copy Markdown
Contributor

@id-ms id-ms left a comment

Choose a reason for hiding this comment

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

LGTM

@id-ms id-ms requested a review from winder September 8, 2022 12:53
@id-ms
Copy link
Copy Markdown
Contributor

id-ms commented Sep 8, 2022

@winder, is it necessary to add "408 Request Timeout " to algod.oas3.json?

Comment thread daemon/algod/api/server/v2/handlers.go Outdated
}

stateProof, err := GetStateProofTransactionForRound(ledger, basics.Round(round), ledger.Latest())
stateProof, err := GetStateProofTransactionForRound(ledger, basics.Round(round), ledger.Latest(), time.Minute, v2.Shutdown)
Copy link
Copy Markdown
Contributor

@winder winder Sep 8, 2022

Choose a reason for hiding this comment

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

Using a context for this is more conventional and doesn't require managing resources, for example context.WithTimeout(ctx.Request().Context()), time.Minute)

What is v2.Shutdown? It's possible echo would cancel its context during a graceful shutdown, so the context solution might handle this case automatically.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We got some inspiration from this code

apparently, if alogd is being signaled to stop the shutdown chan gets a message.
So I think it is good to stop this operation if algod tries to quit.

Comment thread daemon/algod/api/server/v2/utils.go
@winder
Copy link
Copy Markdown
Contributor

winder commented Sep 8, 2022

@winder, is it necessary to add "408 Request Timeout " to algod.oas3.json?

Yes, we should add it to algod.oas2.json or use a 500.

Copy link
Copy Markdown
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants