Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement the UtxoSource interface for REST/RPC clients #2248
Implement the UtxoSource interface for REST/RPC clients #2248
Changes from all commits
8164cb9
01857b5
b315856
3482fce
189c1fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking nit: Technically this does not spawn futures but tasks. :P
One day we may be able to replace that with an
std
trait: rust-lang/wg-async#283There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bleh. if you think its gonna confuse anyone ill change it but i doubt it otherwise.
Yea, rust async is such an MVP...sadly its gotten all hung up on deciding big hairy decisions so never really got past MVP :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a note that since we don't move the block to the end of the queue here, we may pop off a block even though it's been used recently - probably not a big deal, but just something that popped up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think its fine. Making it LRU is possible, but we'd have to shift the array, which isn't bad but is kinda annoying. Also, I don't think the behavior of "LRFirstUsed" is all that bad, either, really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could end up fetching the block multiple times here since we're not holding the lock, but probably not a big deal since we're already ok with the bandwidth costs associated with P2P sync and validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, and we can't hold the lock across an await point. It does suck, I agree, but storing and poll'ing other futures sounded hard 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to call
PeerManager::process_events
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process_events
has to be called cause we now have a pending outbound message waiting to be sent (or possibly we are ready to process more messages from the peer's socket, which we stopped reading from cause we were busy). If we don't call it, the message will go out when we next hit a timer tick in the background processor (or some other regular event calls it for us) but this may be a while.