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

WIP: Run RPC on a separate thread #903

Open
wants to merge 48 commits into
base: development
from

Conversation

@zpalmtree
Copy link
Collaborator

zpalmtree commented Oct 7, 2019

A full RPC rewrite. Runs the RPC on a separate thread.

TODO: Synchronize data races, testing

Resolves: #603

@ExtraHash

This comment has been minimized.

Copy link
Contributor

ExtraHash commented Oct 7, 2019

Wow, cool PR

@zpalmtree zpalmtree force-pushed the zpalmtree:threaded-rpc branch from 4010049 to 2dd48fc Oct 7, 2019
@kryptonchain

This comment has been minimized.

Copy link
Contributor

kryptonchain commented Oct 7, 2019

Well done.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Oct 7, 2019

I'm running into compilation issues on windows after that latest push and it appears that appveyor is as well.

My build was from 2dd48fc last night and I had a lot of exceptions thrown. This is definitely a good idea and a step in the right direction but we need to work to avoid throwing and avoiding race conditions as much as possible.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 7, 2019

I'm running into compilation issues on windows after that latest push and it appears that appveyor is as well.

My build was from 2dd48fc last night and I had a lot of exceptions thrown. This is definitely a good idea and a step in the right direction but we need to work to avoid throwing and avoiding race conditions as much as possible.

Latest commit should fix windows compilation issues. I am going to look more into the race conditions / crashes but am not exactly sure how we're going to manage to selectively synchronize stuff in core without blocking the whole thing.

@zpalmtree zpalmtree changed the title Run RPC on a separate thread WIP: Run RPC on a separate thread Oct 7, 2019
@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Oct 7, 2019

I am going to look more into the race conditions / crashes but am not exactly sure how we're going to manage to selectively synchronize stuff in core without blocking the whole thing.

I hear ya. I'd definitely like to run this under load for a bit longer before this gets merged as the risk may outweigh the benefit in some circumstances.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 7, 2019

I think there's something wrong with /getwalletsyncdata with this change, not seeing it getting hit in the logs via a wallet, but I am seeing it when manually curling it.. will investigate later.

@LeoStehlik

This comment has been minimized.

Copy link
Contributor

LeoStehlik commented Oct 7, 2019

definitely a step into the right direction @zpalmtree thank you for this! I will test wallet sync later, but when curling getwalletsyncdata against (dego) daemon at .1sec cadence, ... even with 0 txs in the pool, I never seen the daemon respond so fast and consistently. Sure it will take time to catch the lose ends, race conditions bugs it may introduce, but I like this already!

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 15, 2019

Full rewrite can be found here: https://github.com/turtlecoin/turtlecoin/compare/development...zpalmtree:threaded-rpc-wip?expand=1

I am not pushing it up to this PR yet as it would break people building from this branch. The branch linked above now has enough endpoints to sync and send transactions with zedwallet-beta.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 27, 2019

Pushed up the full RPC rewrite, with all methods completed that I plan to implement.

I now need to cleanup the NodeRpcProxy code, and figure out how to synchronize access to Core to fix rare crashes.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 27, 2019

Legacy blockchain explorer has been removed. NodeRpcProxy code has been cleaned up.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 28, 2019

This is ready for review and testing. I need to do more testing to try and reproduce crashes, but people helping me with this would not hurt. I didn't manage to reproduce the previous crashes using /info, but have not got around to testing other endpoints.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Oct 28, 2019

This is ready for review and testing. I need to do more testing to try and reproduce crashes, but people helping me with this would not hurt. I didn't manage to reproduce the previous crashes using /info, but have not got around to testing other endpoints.

Roger that. Looks like a bit of stuff to read through :)

Copy link
Collaborator

brandonlehmann left a comment

First pass through and looks pretty good. Just a few comments scattered here and there.

I was quite surprised to see all of the JSON serialization living in RpcServer.cpp -- are those really one-time use for just the RPC like that or is there a case for keeping them elsewhere like previously?

src/rpc/RpcServer.cpp Show resolved Hide resolved
src/rpc/RpcServer.cpp Show resolved Hide resolved
src/rpc/RpcServer.cpp Outdated Show resolved Hide resolved
src/rpc/RpcServer.cpp Outdated Show resolved Hide resolved
@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 29, 2019

I was quite surprised to see all of the JSON serialization living in RpcServer.cpp -- are those really one-time use for just the RPC like that or is there a case for keeping them elsewhere like previously?

I could imagine in the future we may want the JSON serialization to be used elsewhere. However, until that day arrives, I would much rather keep it here in the RpcServer.

My reasoning is as follows:

  • The current serialization code is very hard to follow, and very far away from where it is used
  • We do not yet need this serialization elsewhere
  • We won't save many lines of code by pulling out some of the duplication into methods
  • By the majority of methods in the RPC being simply making a core call, then looping through the data and writing it, it is both very simple to read and follow, and also super easy to work out the data structure returned without having to manually run the calls, some of which have conditional data.
@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Oct 29, 2019

Does this solve #603 in the process?

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 29, 2019

Does this solve #603 in the process?

Almost certainly. I will verify later today.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Oct 30, 2019

Tested - Yep, it fixes it.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Nov 3, 2019

/getblocks returns the raw blocks. On second thought, can we keep that one too?

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 4, 2019

/getblocks returns the raw blocks. On second thought, can we keep that one too?

OK, will update

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 4, 2019

Also, does anything use '/getblocks' right now? I wouldn't mind modernizing it if not since I plan to use it.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Nov 4, 2019

Nothing yet that I’m aware of. Feel free to update it a bit.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 4, 2019

Added /getrawblocks, functions nearly identically to /getwalletsyncdata, only returning the raw block + transactions in hex. Using a new endpoint to avoid confusion when communicating with both old and new daemons.

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

brandonlehmann commented Nov 4, 2019

Added /getrawblocks, functions nearly identically to /getwalletsyncdata, only returning the raw block + transactions in hex. Using a new endpoint to avoid confusion when communicating with both old and new daemons.

Thanks. Will check it out.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 5, 2019

I think there is an issue with /get_global_indexes_for_range not always returning the right data. Furthermore, I think there is a bug where if this fails, the whole sync process comes to a halt.

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 5, 2019

Need to add user agent to request logging

@zpalmtree

This comment has been minimized.

Copy link
Collaborator Author

zpalmtree commented Nov 11, 2019

User agent added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.