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

Add async script RFC #868

Merged
merged 3 commits into from Aug 21, 2020
Merged

Add async script RFC #868

merged 3 commits into from Aug 21, 2020

Conversation

hdevalence
Copy link
Contributor

@hdevalence hdevalence commented Aug 10, 2020 •
edited

@zfnd-bot zfnd-bot bot added this to In progress in 🦓 Aug 10, 2020
yaahc
yaahc reviewed Aug 10, 2020
View reviewed changes
Copy link
Contributor

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

Looks good, I just have a couple of questions asking for clarification.

book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
@hdevalence hdevalence mentioned this pull request Aug 10, 2020
Closed
teor2345
teor2345 requested changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for this work.

I have some questions about moving UTXOs from memory to disk, and some suggestions for handling timeouts and cancellation.

How will we test this implementation?
(Should we add a testing section to all our RFCs?)

book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
🦓 automation moved this from In progress to Review in progress Aug 11, 2020
dconnolly
dconnolly previously approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

solid👍

book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
book/src/dev/rfcs/XXXX-asynchronous-script-verification.md Show resolved Hide resolved
@yaahc yaahc added A-docs Area: Documentation C-design Category: Software design work Poll::Ready labels Aug 11, 2020
@yaahc @dconnolly
Update book/src/dev/rfcs/XXXX-asynchronous-script-verification.md
7194e1b 
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
@hdevalence @yaahc
Remove check_block
2afc208 
Per discussion where @yaahc suggested that it would be simpler to delete this function entirely and treat it as an implementation detail.

Co-authored-by: Jane Lusby <jlusby42@gmail.com>
yaahc
yaahc approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

I think this is good to go once teor's comments are fully resolved, I'll leave it to them to mark it when they're satisfied.

@hdevalence
Copy link
Contributor Author

One additional thing I'd like to nail down before we merge this is the way that we feed this information into the interface to zcash_consensus. It would be unfortunate if we implemented this design and then discovered that things don't quite line up.

@hdevalence hdevalence mentioned this pull request Aug 11, 2020
Merged
@hdevalence
Copy link
Contributor Author

Blocked on #708.

@dconnolly dconnolly mentioned this pull request Aug 11, 2020
Closed
27 tasks
teor2345
teor2345 approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

My questions have been answered.

🦓 automation moved this from Review in progress to Reviewer approved Aug 12, 2020
@yaahc

This comment has been minimized.

Sign in to view
@yaahc
Copy link
Contributor

yaahc commented Aug 17, 2020

#708 should be ready for final review and merge now

@dconnolly
Copy link
Contributor

#708 has been merged, is this now unblocked or do we still have to decide on the points in #868 (comment) ?

@yaahc
Copy link
Contributor

yaahc commented Aug 21, 2020

@dconnolly that comment is resolved now so I think that this RFC should be good to go.

@dconnolly dconnolly merged commit dda0d2d into main Aug 21, 2020
🦓 automation moved this from Reviewer approved to Done Aug 21, 2020
@dconnolly dconnolly deleted the async-script branch August 21, 2020 20:38
@hdevalence hdevalence mentioned this pull request Aug 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dconnolly dconnolly dconnolly approved these changes

@yaahc yaahc yaahc approved these changes

@teor2345 teor2345 teor2345 approved these changes

Assignees
No one assigned
Labels
A-docs Area: Documentation C-design Category: Software design work
Projects
No open projects
🦓
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hdevalence @yaahc @dconnolly @teor2345