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

Reveal pre-images on a timer #860

Merged
merged 2 commits into from May 26, 2020
Merged

Reveal pre-images on a timer #860

merged 2 commits into from May 26, 2020

Conversation

TrustHenry
Copy link
Member

@TrustHenry TrustHenry commented May 20, 2020

Described in each commit.

Relates to #582

@TrustHenry
Copy link
Member Author

I should have pressed Draft PR, but I couldn't. :(

@Geod24
Copy link
Collaborator

Geod24 commented May 20, 2020

You can change it :)

@TrustHenry TrustHenry marked this pull request as draft May 20, 2020 08:25
@AndrejMitrovic
Copy link
Contributor

With regards to the timer implementation, you will need to first submit a PR to LocalRest with a test-case.

@MichaelKim20
Copy link
Member

I'd like to PR https://github.com/Geod24/localrest/. It would be convenient to review and fix.

source/agora/common/Task.d Outdated Show resolved Hide resolved
@TrustHenry TrustHenry force-pushed the timer branch 7 times, most recently from 7bd84f7 to 2b2cc99 Compare May 26, 2020 04:34
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #860 into v0.x.x will decrease coverage by 0.26%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x     #860      +/-   ##
==========================================
- Coverage   90.44%   90.17%   -0.27%     
==========================================
  Files          70       70              
  Lines        5569     5570       +1     
==========================================
- Hits         5037     5023      -14     
- Misses        532      547      +15     
Flag Coverage Δ
#integration 53.01% <55.55%> (+0.57%) ⬆️
#unittests 88.48% <20.00%> (-0.35%) ⬇️
Impacted Files Coverage Δ
source/agora/node/FullNode.d 84.37% <ø> (-5.49%) ⬇️
source/agora/test/EnrollmentManager.d 96.87% <ø> (-0.35%) ⬇️
source/agora/node/Validator.d 72.41% <42.85%> (-9.41%) ⬇️
source/agora/common/Task.d 100.00% <100.00%> (ø)
source/agora/test/Base.d 85.20% <100.00%> (+0.06%) ⬆️
source/agora/network/NetworkClient.d 88.88% <0.00%> (-5.56%) ⬇️
source/agora/network/NetworkManager.d 75.43% <0.00%> (-1.76%) ⬇️
source/agora/utils/PrettyPrinter.d 89.28% <0.00%> (-0.90%) ⬇️
source/agora/consensus/ValidatorSet.d 88.03% <0.00%> (-0.86%) ⬇️

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 7e935bf...794c9f3. Read the comment docs.

@TrustHenry TrustHenry force-pushed the timer branch 2 times, most recently from 571e8dd to c8363a3 Compare May 26, 2020 06:18
@TrustHenry TrustHenry changed the title [WIP] Reveal pre-images on a timer Reveal pre-images on a timer May 26, 2020
@TrustHenry TrustHenry marked this pull request as ready for review May 26, 2020 06:24
@AndrejMitrovic
Copy link
Contributor

Actually I am a little bit confused about vibe.d. It claims it supports null delegates, but it has two overloads of setTimer. One which takes a nothrow and which can be null, and another which takes a throwable delegate which it calls without checking for null.

It's probably an oversight? Hmm.

@AndrejMitrovic
Copy link
Contributor

I think maybe we should just reject null delegates? I don't know of any use-case for it.

@TrustHenry
Copy link
Member Author

https://github.com/vibe-d/vibe-core/blob/872bff8472cc6755e4d0424b347a19a4f5d86ec3/source/vibe/core/core.d#L873-L883
I've referred here.
But as @AndrejMitrovic mentioned, we don't have to allow null delegate yet.

@AndrejMitrovic
Copy link
Contributor

I've referred here.

Yes but your code doesn't call that overload. It calls this one:

https://github.com/vibe-d/vibe-core/blob/872bff8472cc6755e4d0424b347a19a4f5d86ec3/source/vibe/core/core.d#L906

@TrustHenry
Copy link
Member Author

That's right. It's overloaded.
We'd better not allow null delegate.

@AndrejMitrovic
Copy link
Contributor

There is an existing networking test which tests that preimages are revealed when a transaction is sent. That test should now be rewritten.

@TrustHenry
Copy link
Member Author

The existing issue comments were deleted and the test was simplified

Copy link
Collaborator

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Broadly LGTM

source/agora/common/Task.d Outdated Show resolved Hide resolved
source/agora/common/Task.d Outdated Show resolved Hide resolved
source/agora/test/Base.d Outdated Show resolved Hide resolved
source/agora/common/Task.d Outdated Show resolved Hide resolved
source/agora/test/EnrollmentManager.d Show resolved Hide resolved
@TrustHenry
Copy link
Member Author

@bpfkorea/core Is 10 seconds appropriate for my suggested interval of Reveal pre-images?

@Geod24
Copy link
Collaborator

Geod24 commented May 26, 2020

I think it's a good start

@linked0
Copy link
Contributor

linked0 commented May 26, 2020

I'll add a task item about making the period for revalation of pre-image configurable in the issue #520 .

We need a timer function in the operating or test environment.
This function is delegate called repeatedly or after a certain time.
The having pre-image  is currently being checked in FullNode.putTransaction
 to see if pre-image is required.
It's call location is inefficient And that function is only needed by the validator.
Therefore, it is checked using a timer that is called periodically.
Copy link
Collaborator

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

LGTM. Merge at will.

@Geod24 Geod24 merged commit 982ad9e into bosagora:v0.x.x May 26, 2020
@TrustHenry TrustHenry deleted the timer branch May 26, 2020 09:44
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.

None yet

5 participants