HathorNetwork / rfcs

Request for Comments focused on Hathor Network
Apache License 2.0
10 stars 8 forks source link

[Design] API for Transaction Mining #24

Closed jansegre closed 4 years ago

jansegre commented 4 years ago

Phase 2 of our design to reduce costs involves creating an API for mining transactions that is independent from the main API.

This will require a change in the transaction execution flow on the wallet and ultimately require 3 major components: the tx mining server, new fullnode API calls, wallet update to use the new flow.

Wallet changes

Steps to build and publish a transaction on the wallett are currently:

With the new tx-mining API:

Changes to the fullnode API

We will need to create an API to get fresh parents to use on a new transaction. Alternatively and additionally the fullnode could pre-validate a transaction (and fill-in the parents if needed).

The Transaction Mining Server

To summarize, the layout is:

 +-------+
 |Wallets|
 +-------+
     |
     |      +------------------+
     v      |                  |
 HTTP API+--+ Tx Mining Server +---+Stratum
            |                  |       ^
            +------------------+       |
                                       |
                                   +------+
                                   |Miners|
                                   +------+

The HTTP API should look like this (specific formats, endpoint names, field names are up to change):

The reasons for this API to be modeled in a way that the client has to make multiple requests are:

Future improvements

obiyankenobi commented 4 years ago

On /submit-job, what data did you plan to send? We need to make sure people can't abuse or attack the tx miners, possibly by sending a malformed or invalid tx.

msbrogli commented 4 years ago

It seems good to me, but I think we should give at least one example of the requests and responses. We should also add a throttling to both endpoints.

jansegre commented 4 years ago

On /submit-job, what data did you plan to send? We need to make sure people can't abuse or attack the tx miners, possibly by sending a malformed or invalid tx.

Today we won't be any worse off by receiving the same data we receive on the push-tx endpoint

It seems good to me, but I think we should give at least one example of the requests and responses.

I'll work on adding and example.

We should also add a throttling to both endpoints.

We should use the same nginx throttling strategy as used on fullnode APIs.

pedroferreira1 commented 4 years ago

For me it's good.

Only one thing was not 100% clear. After sending the first request (/submit-job) the wallets will need to check from time to time if the job is already finished, right?

If that's the case we should include this UI change in the wallet, so we can give a more precise feedback (Solving tx pow. ETA X. Then move to 'Propagate tx to the network'. Then success).

pedroferreira1 commented 4 years ago

Another thing that should be discussed is what will be done with the current thin_wallet/send_tokens API. We should deprecate it but keep compatibility because old wallets might still be using it. And as @jansegre said

We will connnect miners to the Tx mining server stratum, which will replace our only practical use for the Stratum built-in on the fullnode.

When we decide that it's good to remove the compatibility with the old API we can think about removing the Stratum built-in on the fullnode.

obiyankenobi commented 4 years ago

We will connnect miners to the Tx mining server stratum, which will replace our only practical use for the Stratum built-in on the fullnode.

Nowadays we still have the stratum api available for miners. I'm not sure how many people are using it, but we need to check that before we can remove it.

jansegre commented 4 years ago

Nowadays we still have the stratum api available for miners. I'm not sure how many people are using it, but we need to check that before we can remove it.

@pedroferreira1 @obiyankenobi I created a card to track the deprecation of the built-in stratum

Another thing that should be discussed is what will be done with the current thin_wallet/send_tokens API.

@pedroferreira1 As currently proposed in HathorNetwork/hathor-core#67 self-mining should be deprecated after a while. I added checkboxes to help track the progress. But I think we can either discus specific detals there or on a new issue, and keep this one just for the design of this new system.

obiyankenobi commented 4 years ago

With the new tx-mining API:

  • generate tx locally (outputs and inputs without signature)
  • [request to fullnode API]: get parents (new API call)
  • sign inputs
  • [request to tx-mining API]: create tx mining job (since it returns a time estimate this can be used - for UI feedback)

Parents are not part of the signature, so we don't need to get them before contacting the mining server. The full node could add them as part of /push-tx.

pedroferreira1 commented 4 years ago

Parents are not part of the signature, so we don't need to get them before contacting the mining server. The full node could add them as part of /push-tx.

Yes, we don't need the parents for the signature but the /push-tx expects the tx already mined (with the nonce). And to mine the tx we need the parents, so we need to add the parents before sending to the mining server, I think.

@pedroferreira1 As currently proposed in HathorNetwork/hathor-core#67 self-mining should be deprecated after a while. I added checkboxes to help track the progress. But I think we can either discus specific detals there or on a new issue, and keep this one just for the design of this new system.

Ok, I agree with you. We should discuss the deprecation in another issue

For me the design is good to close. Can you just confirm this question, please?

Only one thing was not 100% clear. After sending the first request (/submit-job) the wallets will need to check from time to time if the job is already finished, right?

msbrogli commented 4 years ago

I like the design, but I think we should get into the pooling strategy. Should we check the status every X seconds, where X is half the expected mining time?

I also think we can discuss the changes in both wallets. Today, after clicking on Send Tx, the user sees a "Sending..." screen. I think we should split it into two screens: "Signing tx...", then "Resolving tx...", and finally, "Sending tx...".

In the "Resolving tx..." we can use the expected time to fill a progress bar. The progress bar can grow from 0% to 100% linearly on max(2, 2*X seconds) (where X is the expected time). While it is growing, we should check for the mining status every max(0.5, X/2) seconds.

obiyankenobi commented 4 years ago

Yes, we don't need the parents for the signature but the /push-tx expects the tx already mined (with the nonce). And to mine the tx we need the parents, so we need to add the parents before sending to the mining server, I think.

We could use some other API instead of /push-tx, like the one we currently use for sending tokens. But why do we need the parents to mine a tx?

I think we should split it into two screens: "Signing tx...", then "Resolving tx...", and finally, "Sending tx...". What's each step? I understand signing and sending, but what's resolving?

Also, while I think we could have a progress bar, I'd leave it for future improvements.

pedroferreira1 commented 4 years ago

We could use some other API instead of /push-tx, like the one we currently use for sending tokens. But why do we need the parents to mine a tx?

The parents are part of the graph hash used to calculate the tx hash. We could discuss about removing it from there but I think this should be done elsewhere. Right now we need the parents before sending to the mine API, so we can use our current /push-tx endpoint after returning from the mining.

Also, while I think we could have a progress bar, I'd leave it for future improvements.

I agree we could leave for the future but I think we should at least give a feedback of the steps (like we do when using the ledger).

obiyankenobi commented 4 years ago

The parents are part of the graph hash used to calculate the tx hash. We could discuss about removing it from there but I think this should be done elsewhere.

I completely mixed things up. They are not in the signature but we do need them for mining. My bad.

jansegre commented 4 years ago

Parents are not part of the signature, so we don't need to get them before contacting the mining server. The full node could add them as part of /push-tx.

Yes, as @pedroferreira1 said, they are needed for mining.

For me the design is good to close. Can you just confirm this question, please?

Only one thing was not 100% clear. After sending the first request (/submit-job) the wallets will need to check from time to time if the job is already finished, right?

Yes, the wallet will need to do some polling to get the result.

I like the design, but I think we should get into the pooling strategy. Should we check the status every X seconds, where X is half the expected mining time?

I also think we can discuss the changes in both wallets. Today, after clicking on Send Tx, the user sees a "Sending..." screen. I think we should split it into two screens: "Signing tx...", then "Resolving tx...", and finally, "Sending tx...".

In the "Resolving tx..." we can use the expected time to fill a progress bar. The progress bar can grow from 0% to 100% linearly on max(2, 2*X seconds) (where X is the expected time). While it is growing, we should check for the mining status every max(0.5, X/2) seconds.

I like discussing UI/UX, but I think we could do it directly on the pull request of the respective wallets. Mainly because we could opt for the faster implementation, and also because the current proposal seems to already supports a wide range of UI/UX options.

About the polling strategy, I'll add that as the recommended strategy.

obiyankenobi commented 4 years ago

This is good for me.

As Jan mentioned, we should debate wallet UI/UX on a separate issue on each wallet.

pedroferreira1 commented 4 years ago

Good for me!

msbrogli commented 4 years ago

Good for me as well.