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

Fullnode RPC and pub/sub need enhancement #2044

Closed
boqiu opened this issue Jan 18, 2021 · 13 comments
Closed

Fullnode RPC and pub/sub need enhancement #2044

boqiu opened this issue Jan 18, 2021 · 13 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@boqiu
Copy link
Contributor

boqiu commented Jan 18, 2021

  1. Some local and test RPCs (e.g. txpool related RPCs) are very useful for scan and infuradevelopment, suggest fullnode to allow user to configure public/intranet access for those RPCs, just as what Ethereum did now.
  2. Need some enhancements for current PRCs and pub/sub for infura development:
    • RPC: some data missed in the returned logs of RPC cfx_getTransactionReceipt.
    • RPC: better to provide a public or local API to return receipts for specified block or even epoch number, e.g. getBlockReceipts(blockHash), getEpochReceipts(epoch), it will significantly increase the performance of data sync.
    • Pub/sub: atomically push batch epochs to client when pivot chain switched, which is handled in a lock on fullnode. Thus, client could also atomically handle pivot chain switch.
    • Pub/sub: allow user to specify which epoch to subscribe, e.g. latest_mined, latest_state, latest_confirmed or latest_checkpoint.
    • RPC & Pub/sub: support to return 4 epoch numbers, including latest_mined, latest_state, latest_confirmed or latest_checkpoint.
@boqiu boqiu added enhancement New feature or request help wanted Extra attention is needed labels Jan 18, 2021
@resodo
Copy link
Contributor

resodo commented Jan 18, 2021

Thanks, I agree with most of them. The Pub/Sub part is not familiar to me, @Thegaram please advise it

Some local and test RPCs (e.g. txpool related RPCs) are very useful for scan and infura development, suggest fullnode to allow user to configure public/intranet access for those RPCs, just as what Ethereum did now.

Agree. But Some test RPCs are too much expensive, which should really carefully to expose it.

RPC: some data missed in the returned logs of RPC cfx_getTransactionReceipt.

Could you specify them all?

RPC: better to provide a public or local API to return receipts for specified block or even epoch number, e.g. getBlockReceipts(blockHash), getEpochReceipts(epoch), it will significantly increase the performance of data sync.

sounds reasonable. Maybe we can add cfx_getReceiptsByBlockHash.

Pub/sub: atomically push batch epochs to client when pivot chain switched, which is handled in a lock on fullnode. Thus, client could also atomically handle pivot chain switch.

Sounds reasonable to me, the protocol should be carefully designed.

Pub/sub: allow user to specify which epoch to subscribe, e.g. latest_mined, latest_state, latest_confirmed or latest_checkpoint.

Sounds reasonable to me.

RPC & Pub/sub: support to return 4 epoch numbers, including latest_mined, latest_state, latest_confirmed or latest_checkpoint.

I am not clear, return 4 epoch numbers on what scenarios?

@boqiu
Copy link
Contributor Author

boqiu commented Jan 18, 2021

1)It's up to user to decide which RPCs to open, at least in an intranet.
2) The logs returned by cfx_getTransactionReceipt only contains Address, Data and Topics, other fields are all missed.

@resodo
Copy link
Contributor

resodo commented Jan 18, 2021

Also, for the scan, the atomicity of RPCs is really important. So we have getXXXWithPivotAssumption to handle it. If you want some RPCs just for scan syncing service. The atomicity is a vital thing to handle.

@resodo
Copy link
Contributor

resodo commented Jan 18, 2021

And the name of test RPCs is still to be finalized. Maybe it's better to finalize it before make it optional to expose.

Could you specify which of them you want the most?

@boqiu
Copy link
Contributor Author

boqiu commented Jan 18, 2021

At least, txpool related RPCs are required for scan to display pending txs.

@Thegaram
Copy link
Contributor

RPC: some data missed in the returned logs of RPC cfx_getTransactionReceipt.

The log structure in cfx_getTransactionReceipt and cfx_getLogs is different to avoid duplication. All the data is available in the receipts response, with the exception of logIndex (i.e. the index of a log within the epoch). If we support cfx_getReceiptsByBlockHash, this info should be very easy to derive.

As for a cfx_getReceiptsByBlockHash RPC, Parity has something similar, it should be very easy to support on Conflux.

@Thegaram
Copy link
Contributor

I am still a bit confused about the atomicity issue of pubsub. If you want to retrieve block data, you can use cfx_getBlockByHashWithPivotAssumption as mentioned which will return null if there was a chain reorg. Could you please add a specific examples on where the lack of atomicity in pubsub causes issues?

@boqiu
Copy link
Contributor Author

boqiu commented Jan 18, 2021

@Thegaram just want to keep consistency for public PRC between infura and fullnode

@resodo
Copy link
Contributor

resodo commented Jan 18, 2021

At least, txpool related RPCs are required for scan to display pending txs.

The pending transaction is not that trivial. Tx-pools can contain at most 100,000 transactions. There's no way to display pending transactions on ConfluxScan by just exposing some debug RPCs like "inspect_pool" due to such a large amount of data, and the paging and indexing could be needed.

And, due to a much lower pending period on Conflux that BTC or ETH. Personally, I think the pending transaction is good to have, with a lower priority.

@boqiu
Copy link
Contributor Author

boqiu commented Jan 18, 2021

@resodo in my option, scan could provide a way for a single user to query the pending txs on demand, especially for txs that pended for a long time, e.g. 5 minutes. In this case, full node could provide a API with enough performance. Anyway, we could have more discussion from the productivity's point of view.

@Thegaram
Copy link
Contributor

Thegaram commented Feb 8, 2021

RPC & Pub/sub: support to return 4 epoch numbers, including latest_mined, latest_state, latest_confirmed or latest_checkpoint.

Did some thinking about this. What's the utility of getting these in one request as opposed to four parallel requests? Is it just that it's cheaper? Consistency/atomicity does not seem to be an important concern here.

@boqiu
Copy link
Contributor Author

boqiu commented Feb 8, 2021

Yes, just for cheaper. Anyway, it's OK to get 4 values via current RPC cfx_getEpochNumber.

@Thegaram
Copy link
Contributor

Thegaram commented Apr 6, 2021

Most of these have been implemented. Please reopen if there are any missing features you need.

@Thegaram Thegaram closed this as completed Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants