-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add ability for MPW to run explorer-less #427
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cheery-moxie-4f1121 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Left some comments. Overall my idea is that we should make sure to have working explorers instead of making the code more ugly/complicated just to use RPC nodes as fallback.
For example: ok the explorers are not working and I can get the blockCount
from the RPC node, but then getLatestTxs
is going to fail anyways because it can only be done by an explorer, so what's the point?
scripts/network.js
Outdated
const newTx = await r.json(); | ||
newTxs.push(newTx); | ||
} else { | ||
newTxs.push(tx); |
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.
this is very good, we should consider removing the explorer based getBlock
scripts/wallet.js
Outdated
await cNet.getLatestTxs(this); | ||
fSynced = true; | ||
} catch { | ||
// If all Explorers are down, we'll just rely on the local TXDB and display a warning |
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.
I think this is more serious: getLatestTxs
failed, wallet is almost unusable, balance is wrong and any attempt to create a tx will likely fail.
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.
Shield is fully usable and transactable, as well as Masternode controller and Governance. Meanwhile current MPW will not even open/load if explorers are down (which they are down far too frequently).
The idea of the warning anyway is that as soon as the getLatestTxs
API is available again - we want to re-run getLatestTxs
and then we can drop the warning (just not sure where to handle this state yet).
return backend.bestBlockHash; | ||
return backend.bestBlockHash; | ||
} catch { | ||
// Use Nodes as a fallback |
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.
I don't like using nodes as a fallback for light RPC calls like getBestBlockHash
, sendTransaction
and getBlockCount
: the code is more ugly and imo we should just make sure to have working explorers
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.
Imo we would need at least 2x (4) explorers to consider it a stable network, or we start relying more on Nodes instead, it is the only compromise to get MPW more stable.
await this.#transparentSync(); | ||
} catch { | ||
// We'll set a 5s interval sync until it's finally successful, then nuke the 'thread'. | ||
const cThread = new AsyncInterval(async () => { |
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.
You cannot use AsyncInterval
: the function sync()
has to be blocked until transparentSync()
is succesful, otherwise we end up in a state where the wallet is marked as synced, this.#isSynced = true
, even if it isn't.
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.
The issue with blocking it is Shield then becomes unusable unless Transparent syncs (right now, Shield can be used even if Transparent is unable).
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.
Ideally we would split the state so that we have transparentSynced
and shieldSynced
, so they are operable independently.
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.
tACK, left two very minor nits.
async #transparentSync(force = false) { | ||
if ((!this.isLoaded() || this.#isSynced) && !force) return; |
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.
Move the loadedCheck and synced in the sync function, so we can remove the force param.
async #transparentSync(force = false) { | |
if ((!this.isLoaded() || this.#isSynced) && !force) return; | |
async #transparentSync() { |
It's only going to be called by sync anyways
{ current: totalPages - i + 1 }, | ||
{ total: totalPages }, | ||
]); | ||
(i, totalPages, finished, message) => { |
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.
(i, totalPages, finished, message) => { | |
(i, totalPages, finished, error) => { |
Can you call this error/warning?
Abstract
This PR allows the majority of MPW data, aside from heavily indexed (Pubkey/XPub) data, to be acquired from Node RPCs.
This was initially as a fix for MPW being completely unloadable when Explorers are down, due to being unable to fetch chain data, MPW can't even pass the "Loading" screen - however after getting in to the rabbit hole, this PR is able to replace the reliance on Explorers almost entirely and allow full access to MPW despite total explorer outage, relying instead on the internal TXDB, internal mempool, and public RPC data.
This PR adds the ability to use all MPW functions aside from Transparent Sync - Shield can be fully utilised as normal.
RPC Layer Suggestions
This PR proposes the following RPCs be enabled on all RPC Servers supported by MPW:
getblockcount, getblockhash, getblock, getbestblockhash, getrawtransaction, sendrawtransaction
It would however be more efficient to develop a custom RPC layer API like
getblockbyheight
, it would just begetblockhash
passed throughgetblock
, halving the requests MPW needs to make for blockchain sync-via-RPC.TODOs
Shield
is synced and ready, butTransparent
is not.Testing
To test this PR, it's suggested to attempt these user flows, or variations of these:
If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!