-
Notifications
You must be signed in to change notification settings - Fork 91
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
client/asset/eth: add RPC client #1832
Conversation
buck54321
commented
Aug 30, 2022
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.
Looks really nice, but I was unable to get it to work on simnet. I used the ipc file to add a provider, and at first it says it has connected to a peer, but when I try to make a trade, it fails saying that it has no peers.
Also, there is a UI issue. If I have a native wallet already created, I cannot see the Recieve/Send buttons, and I cannot switch to the rpc wallet. When I have no wallets created, I do not see the option of creating a native wallet.
} | ||
|
||
m.receipts.Lock() | ||
m.receipts.cache[txHash] = &receiptRecord{ |
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 we shouldn't cache until the receipt has a certain number of confirmations. If we cache in the first block it is mined, then there is a reorg, it could cause issues.
client/asset/eth/multirpc.go
Outdated
lastProvider = p | ||
m.log.Tracef("Sending signed tx via %q", p.host) | ||
err := p.ec.SendTransaction(ctx, tx) | ||
if err != nil && strings.Contains(err.Error(), core.ErrAlreadyKnown.Error()) { |
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 not errors.Is
instead of the strings.Contains
?
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.
Coming through the (rpc.Client).CallContext
, we lose the type.
} | ||
|
||
func (m *multiRPCClient) syncProgress(ctx context.Context) (prog *ethereum.SyncProgress, err error) { | ||
return prog, m.withAny(func(p *provider) error { |
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.
What if some providers are synced and some are not? Should we just return 100% synced progress, and in the other methds use the providers that have already finished syncing?
@@ -21,6 +21,13 @@ | |||
<input type="date" class="form-control select"> | |||
</div> | |||
</div> | |||
<div data-tmpl="repeatableInput" class="w-100"> |
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 should have an option to remove additional rows.
I think I've resolved some things now @martonp, if you're up for another spin.
Oh. Interesting. Cuz I've removed support for native wallets. I'll figure this out. |
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.
All the functionality seems to be working well now, except for the redemption confirmation which works after fixing getTransaction
.
client/asset/eth/multirpc.go
Outdated
return err | ||
} | ||
tx = resp.tx | ||
if resp.BlockNumber != nil { |
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 resp.BlockNumber
string has a "0x" in the front, causing the hex.DecodeString
call to return an error. Also this function should return -1 for the height if the tx has not yet been mined.
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.
partial review
client/asset/eth/eth.go
Outdated
providers, err := connectProviders(ctx, endpoints, createWalletParams.Logger, big.NewInt(chainIDs[createWalletParams.Net])) | ||
if err != nil { | ||
return err | ||
} |
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.
Feels weird to have to connect in create, but I see it runs some initial checks. Still maybe could be done in the first Connect. We have a ctx arg there and not here intentionally.
// use the same provider if < nonceProviderStickiness has passed. | ||
var nonceProviderStickiness = time.Minute | ||
|
||
// TODO: Handle rate limiting? From the docs: |
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.
Which docs?
ws bool | ||
tipCapV atomic.Value // *cachedTipCap | ||
|
||
// tip tracks the best known header as well as any error encount |
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.
encount -> encountered
stale := time.Second * 10 | ||
if p.ws { | ||
stale = time.Minute * 2 | ||
} |
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 longer for websocket? (is ws websocket)
Oh, because subscription maybe?
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.
Right. We expect them to come through the subscription.
// failed will be true if setFailed has been called in the last failQuarantine. | ||
func (p *provider) failed() bool { |
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.
Also true if the provider has failed > 100 times.
if strings.Contains(errStr, s) { | ||
return true | ||
} |
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.
Probably fine, but if mi is not a string or error s will always be "" and have a true 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.
That's true, and I think I'm okay with it. I would understand if anyone was adamant that I do it different though.
} | ||
if len(readyProviders) == 0 { | ||
// Just try them all. | ||
m.log.Tracef("all providers in a failed state, so acting like none are") |
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.
Maybe this is a warn?
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 would be somewhat common when using a single provider.
client/asset/eth/multirpc.go
Outdated
if superError == nil { | ||
superError = err | ||
} else { | ||
superError = fmt.Errorf("%v: %w", superError, err) | ||
} |
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.
Not sure if this maybe returns below before the error log, but could add info about the provider to this error?
It seems like the error is just ignored sometimes. Could always log anyway? Debug log even if it's not really a problem?
|
||
for _, p := range m.providers { | ||
if lastProvider != nil && lastProvider.host == p.host { | ||
continue // already added it |
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.
Is comment correct? Looks like will add later. Or rather add everything else to it.
client/asset/eth/multirpc.go
Outdated
// This block choosing algo is probably too rudimentary. Really need | ||
// shnuld traverse parents to a common block and sum up gas (including | ||
// uncles?), I think. |
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.
shnuld -> should
Why is GasUsed is in the equation?
We don't need to worry about uncles as they will not exist anymore, according to reddit https://www.reddit.com/r/ethstaker/comments/vwdki8/will_ommeruncle_blocks_exist_after_the_merge/
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.
Isn't GasUsed the rough equivalent of work in ethereum? I'm not clear on how that works I guess.
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'm not either. It may be that more gas used has more weight.
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 wouldn't guess that gas used has any thing to do with it either. Not saying it doesn't, but are there any references on this?
return nil | ||
} | ||
if propagate { | ||
return err |
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.
Maybe intended but this eats superError
if fail { | ||
p.setFailed() | ||
} |
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.
May be misreading, but if there were two providers and the first one errored and took this fail path while the second one succeeded then method would return the non nil superError
although succeeding with the second provider.
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 had that messed up. Thanks.
// depending on what they are. Zero or more acceptabilityFilters can be added | ||
// to provide extra control. | ||
// | ||
// discard: If a filter indicates discard = true, the error will be discarded, |
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.
nit: extra space in front of discard.
func (m *multiRPCClient) sendSignedTransaction(ctx context.Context, tx *types.Transaction) error { | ||
var lastProvider *provider | ||
if err := m.withPreferred(func(p *provider) error { | ||
lastProvider = p | ||
m.log.Tracef("Sending signed tx via %q", p.host) | ||
return p.ec.SendTransaction(ctx, tx) | ||
}, func(err error) (discard, propagate, fail bool) { | ||
return errorFilter(err, core.ErrAlreadyKnown, "known transaction"), false, false | ||
}); err != nil { | ||
return err | ||
} | ||
m.lastProvider.Lock() | ||
m.lastProvider.provider = lastProvider | ||
m.lastProvider.stamp = time.Now() | ||
m.lastProvider.Unlock() | ||
return nil | ||
} |
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.
It looks like two of these running in parallel could have different providers set in close proximity, possible defeating the purpose of keeping the nonce in order.
// syncProgress: We're going to lie and just always say we're synced if we | ||
// can get a header. |
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.
Could make an exception for IPC?
client/asset/eth/multirpc.go
Outdated
if strings.Contains(err.Error(), "not found") { | ||
notFound = true | ||
} |
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.
Could use the errorFilter
you made.
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.
Can't use it here, because a subsequent provider might find it.
type rpcTest struct { | ||
name string | ||
f func(*provider) error | ||
} |
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.
Testing things could be in a test file?
Maybe unrelated, but seeing for the first time on loadbot:
Seems to have been random as has not happened again. |
if err != nil { | ||
if errors.Is(err, asset.ErrWalletTypeDisabled) { | ||
subject, details := c.formatDetails(TopicWalletTypeDeprecated, unbip(assetID)) | ||
c.notify(newWalletConfigNote(TopicWalletTypeDeprecated, subject, details, db.WarningLevel, nil)) | ||
} | ||
return nil, fmt.Errorf("error opening wallet: %w", err) | ||
} |
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 started into a scheme where we just disable the wallet, but it was a can of worms. Instead, we're refusing to open a deprecated wallet in e.g. eth.NewWallet
and issuing a warning. This is no problem for us right now, because the only other option for Ethereum is also a seeded wallet and will derive the same key. But if there was a need to export a seed from a deprecated wallet, we would need to get more sophisticated here.
panic while testing on testnet using infura:
Unable to recover, keeps panicking when I start up. Other than that working well! |
Also, weird critical error for the server randomly @chappjc:
full logs: |
My bad. Fixed, and I've actually tested it on testnet this time. I've got some orders up on testnet dcr-eth tonight if you get back around to testing. |
Uh. This is kinda nuts. |
client/asset/eth/multirpc.go
Outdated
case err, ok := <-sub.Err(): | ||
if !ok { | ||
// Subscription cancelled | ||
return | ||
} | ||
log.Errorf("%q header subscription error: %v", p.host, err) | ||
log.Info("Attempting to resubscribe to %q block headers", p.host) | ||
sub, err = newSub() | ||
if err != nil { // context cancelled | ||
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.
Trying the nodeclient tests on testnet I will see:
2022-09-23 15:11:34.532 [ERR] ETHTEST: "goerli.infura.io" header subscription error: <nil>
2022-09-23 15:11:34.532 [WRN] ETHTEST: can't resubscribe to "goerli.infura.io" headers: client is closed
2022-09-23 15:11:36.135 [ERR] ETHTEST: "goerli.infura.io" header subscription error: <nil>
2022-09-23 15:11:36.135 [WRN] ETHTEST: can't resubscribe to "goerli.infura.io" headers: client is closed
Expected?
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.
Initial comments. More today.
ec.Close() | ||
log.Errorf("Failed to get header from %q: %v", endpoint, err) | ||
continue | ||
} |
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.
Maybe a ec.HeaderByHash(ctx, hdr.Hash())
check as well now that it's needed?
client/asset/eth/multirpc.go
Outdated
// TODO | ||
// TODO: Plug into the monitoredTx system from #1638. | ||
// TODO | ||
if tx, _, err = m.getTransaction(ctx, txHash); err != nil { | ||
return nil, nil, err | ||
} |
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.
Was this addition intentional? We already have the tx.
If everybody could circle back on their reviews and resolve or bump suggestions, we're nearly ready to get this in. |
https://github.com/decred/dcrdex/pull/1832/files#r965796162 @buck54321 You fixed this in 325a552 but somehow it is back now. |
client/asset/eth: Adds an RPC client that can maintain one or more websocket (preferred), http, or ipc connections. Implements ethFetcher as well as bind.ContractBackend. Websocket connections have a block header feed, so need to make fewer requests. For http connections, the block header requests are metered to one per 10 seconds. If no nonces are involved, the provider to use for a request is picked randomly, but closely grouped requests involving nonces will attempt to use the same provider. client/asset: Add Repeatable bool field to ConfigOption to allow multiple inputs. ui: Work with new repeatable input type.
OK on your reviews, @JoeGruffins? |