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.

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

Flash test: no block at requested height #3042

Merged

Conversation

hewison-chris
Copy link
Contributor

A flash test sometimes fails during gossipChannelsOpen if the call to
getBlock fails. This can happen if the discovered channel height is
greater than the current block height of the peer we attempt to fetch
from. By storing the last block rather than block height we can help
prevent this sort of race condition.

2022-02-14 13:26:59,384 Error [agora.flash.Node] - Exception thrown from `getBlock` while gossipping: geod24.LocalRest.ClientException@submodules/localrest/source/geod24/LocalRest.d-mixin-1242(1262): Request to const(Block) geod24.LocalRest.RemoteAPI!(TestAPI, Serializer).RemoteAPI.getBlock(ulong _param_0) @safe couldn't be processed : vibe.http.common.HTTPStatusException@source/agora/node/FullNode.d(995): No block at requested height

A flash test sometimes fails during `gossipChannelsOpen` if the call to
`getBlock` fails. This can happen if the discovered channel height is
greater than the current block height of the peer we attempt to fetch
from. By storing the last block rather than block height we can help
prevent this sort of race condition.
@hewison-chris hewison-chris self-assigned this Feb 14, 2022
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #3042 (ccf4749) into v0.x.x (83349fd) will increase coverage by 59.10%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           v0.x.x    #3042       +/-   ##
===========================================
+ Coverage   19.83%   78.93%   +59.10%     
===========================================
  Files          61      209      +148     
  Lines        2702    18588    +15886     
===========================================
+ Hits          536    14673    +14137     
- Misses       2166     3915     +1749     
Flag Coverage Δ
integration 19.76% <ø> (-0.07%) ⬇️
unittests 87.36% <84.61%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/flash/Node.d 80.67% <84.61%> (ø)
source/scpd/scp/SCPDriver.d 60.00% <0.00%> (-15.00%) ⬇️
source/scpd/scp/SCP.d 100.00% <0.00%> (ø)
source/agora/flash/OnionPacket.d 97.94% <0.00%> (ø)
source/agora/test/PeriodicCatchup.d 95.45% <0.00%> (ø)
source/agora/test/FutureEnrollment.d 97.56% <0.00%> (ø)
source/agora/crypto/Crc16.d 100.00% <0.00%> (ø)
source/agora/utils/Backoff.d 100.00% <0.00%> (ø)
source/agora/cli/client/GenTxProcess.d 0.00% <0.00%> (ø)
source/agora/crypto/Key.d 98.07% <0.00%> (ø)
... and 150 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 83349fd...ccf4749. Read the comment docs.

@omerfirmak
Copy link
Contributor

I dont think this will help, pushBlock is called by the same Agora node that we query with the getBlock. If getBlock fails, that agora node would have never called pushBlock on our Flash node anyways since it does not have that block.

@hewison-chris
Copy link
Contributor Author

hewison-chris commented Feb 14, 2022

I dont think this will help, pushBlock is called by the same Agora node that we query with the getBlock. If getBlock fails, that agora node would have never called pushBlock on our Flash node anyways since it does not have that block.

But it does happen (see the log line in the description).
Is it not possible that a different node is calling pushBlock to the node we call getBlock in gossipChannelsOpen?

@omerfirmak
Copy link
Contributor

Is it not possible that a different node is calling pushBlock to the node we call getBlock in gossipChannelsOpen?

Ah it is, we configure Agora to push to all Flash nodes (see flashTestConf). That is not how the real world configuration will look like tho. But the race condition is still there, we may receive the channel open event before the block

@hewison-chris hewison-chris enabled auto-merge (rebase) February 14, 2022 07:48
@hewison-chris hewison-chris merged commit edf2052 into bosagora:v0.x.x Feb 14, 2022
@linked0
Copy link
Contributor

linked0 commented Feb 14, 2022

Relates #2425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants