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

eth/server: Problem getting balances. #2016

Closed
JoeGruffins opened this issue Dec 30, 2022 · 10 comments · Fixed by #2023
Closed

eth/server: Problem getting balances. #2016

JoeGruffins opened this issue Dec 30, 2022 · 10 comments · Fixed by #2023
Labels
bug bug or bugfix ETH
Milestone

Comments

@JoeGruffins
Copy link
Member

Seeing the error 2022-12-30 17:20:21.193 [ERR] MKT: (*DEXBalancer).CheckBalance: error getting account balance for %q: %v 0x946dfaB1AD7caCFeF77dE70ea68819a30acD4577 accountBalance error: contentFrom error: the method txpool_contentFrom does not exist/is not available on master. Probably due to a recent geth version change. I do not think I noticed this in the pr, however.

@JoeGruffins
Copy link
Member Author

This is with the simnet harness. Maybe it does need some arguments for what api's are enabled on start up. Looking into it.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Dec 30, 2022

I do not see it on testnet. oh wait, it's failing. Wasn't connecting over ws

@JoeGruffins
Copy link
Member Author

I must be going crazy, I really though this was working before, but it looks like we don't have access to "txpool" methods?

There has been some talk of allowing even paid services for server. In that case there would be no access to the transaction pool I believe. Perhaps we can dumb down this?

// accountBalance gets the account balance, including the effects of known
// unmined transactions.
func (c *rpcclient) accountBalance(ctx context.Context, assetID uint32, addr common.Address) (*big.Int, error) {
tip, err := c.blockNumber(ctx)
if err != nil {
return nil, fmt.Errorf("blockNumber error: %v", err)
}
// We need to subtract and pending outgoing value, but ignore any pending
// incoming value since that can't be spent until mined. So we can't using
// PendingBalanceAt or BalanceAt by themselves.
// We'll iterate tx pool transactions and subtract any value and fees being
// sent from this account. The rpc.Client doesn't expose the
// txpool_contentFrom => (*TxPool).ContentFrom RPC method, for whatever
// reason, so we'll have to use CallContext and copy the mimic the
// internal RPCTransaction type.
var txs map[string]map[string]*RPCTransaction
err = c.caller.CallContext(ctx, &txs, "txpool_contentFrom", addr)
if err != nil {
return nil, fmt.Errorf("contentFrom error: %w", err)
}
if assetID == BipID {
ethBalance, err := c.ec.BalanceAt(ctx, addr, big.NewInt(int64(tip)))
if err != nil {
return nil, err
}
outgoingEth := new(big.Int)
for _, group := range txs { // 2 groups, pending and queued
for _, tx := range group {
outgoingEth.Add(outgoingEth, tx.Value.ToInt())
gas := new(big.Int).SetUint64(uint64(tx.Gas))
if tx.GasPrice != nil && tx.GasPrice.ToInt().Cmp(bigZero) > 0 {
outgoingEth.Add(outgoingEth, new(big.Int).Mul(gas, tx.GasPrice.ToInt()))
} else if tx.GasFeeCap != nil {
outgoingEth.Add(outgoingEth, new(big.Int).Mul(gas, tx.GasFeeCap.ToInt()))
} else {
return nil, fmt.Errorf("cannot find fees for tx %s", tx.Hash)
}
}
}
return ethBalance.Sub(ethBalance, outgoingEth), nil
}

And not worry about the txpool. Using "eth_getBalance" instead which is provided by ethclient.

@chappjc
Copy link
Member

chappjc commented Dec 30, 2022

When I attach via IPC:

instance: Geth/v1.11.0-unstable-a9dfac03-20221207/linux-amd64/go1.19.4
at block: 8229507 (Fri Dec 30 2022 23:44:00 GMT+0000 (UTC))
 datadir: /opt/geth/data
 modules: admin:1.0 clique:1.0 debug:1.0 engine:1.0 eth:1.0 miner:1.0 net:1.0 personal:1.0 rpc:1.0 txpool:1.0 web3:1.0

On the authrpc interface, perhaps not all modules (namely txpool) are available as we thought.

Unfortunately geth attach does not seem to be able to attach with a JWT to the authrpc interface.

How can we list the modules available after connecting to the endpoint?

Anyway, I think server's eth backend needs to support connecting on the unauthenticated http or ws interfaces.

@JoeGruffins
Copy link
Member Author

We have modules: engine:1.0 eth:1.0 rpc:1.0 over authrpc by default. I guess we can't turn on more...

@chappjc
Copy link
Member

chappjc commented Jan 2, 2023

We have modules: engine:1.0 eth:1.0 rpc:1.0 over authrpc by default. I guess we can't turn on more...

That's unfortunate. We'll need to revert all the jwt stuff that targets the authrpc interface, and support the older unauthenticated http/ws interfaces instead. Auth (and transport encryption) are user/operator tasks.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Jan 2, 2023

Are those three not enough? I think if we simplify the server balance we will be fine. Client tests are passing over jwt.

If you want to undo the changes that's fine. I guess you would not rewind master so I can pr it if you wish.

@chappjc
Copy link
Member

chappjc commented Jan 2, 2023

Are those three not enough? I think if we simplify the server balance we will be fine. Client tests are passing over jwt.

  • What is gained from using the authrpc interface with fewer available modules? This interface was added for the engine API for the consensus client. Possibly we shouldn't be using it. (this entire thread: eth: Add authenticated geth rpc capabilities. #1963 (comment) )
  • Is it worth limiting to these three modules when we don't have to?
  • Will the authrpc interface suddenly drop or add modules? Are there guarantees?

I don't know a compelling reason for JWT-authenticated RPC from DEX. It provides no encryption and thus does not prevent an attacker from sniffing the traffic or performing replay attacks (see the engine api auth spec). The authenticated interface is designed to prevent internet exposed services from being used without permission, a newish issue with a separate consensus client that may live on a distant machine. And it creates complications for the dex settings dialogs.

I think a geth user should power dex with either IPC or --http/--ws (and relevant settings pertaining to cors, vhosts, etc.) and if run remotely they should apply transport encryption with solutions like a vpn or ssh tunnel.

@chappjc chappjc added this to the 0.6 milestone Jan 2, 2023
@JoeGruffins
Copy link
Member Author

What is gained from using the authrpc interface with fewer available modules?

An easy way to connect to a remote geth node that the go-ethereum devs will be updating. IF we are allowing things like infura, I don't know why more modules are necessary. I don't think that server needs to be using too many resources on mempool investigation either.

Possibly we shouldn't be using it.

Could be true. Maybe this is why they did not add that commit in a release yet.

Is it worth limiting to these three modules when we don't have to?

Again I don't think we gain much by meddling with the mempool. As you know transactions could be replaced rather easily, erasing earlier mempool transactions. And if we will be using outside services, we are already "limited".

Will the authrpc interface suddenly drop or add modules? Are there guarantees?

I guess not. Your issue will clear this up about how/if we should use it. Obviously that's what I should have done first. I hope it gets a reply...

Can revert in the meantime.

@chappjc
Copy link
Member

chappjc commented Jan 3, 2023

Is it worth limiting to these three modules when we don't have to?

Again I don't think we gain much by meddling with the mempool. As you know transactions could be replaced rather easily, erasing earlier mempool transactions. And if we will be using outside services, we are already "limited".

The main thing we were doing with txpool was to actively subtract pending sends/withdraws so that server won't see a higher balance than really exists if the user has just sent all their funds out. We couldn't use PendingBalanceAt for this because that includes pending receives/deposits too, which as you said shouldn't be trusted.

It is a good point that if we want to support third party RPC providers that perhaps the txpool API may not be available. I would assume it would be, but perhaps that is wrong. Assume a provider does not give those methods, dex can fall back to BalanceAt, ignoring pending, if absolutely necessary.

EDIT: just noting that this is all best-effort, and nothing that is really critical to trust IMO

@chappjc chappjc added ETH bug bug or bugfix labels Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bug or bugfix ETH
Projects
None yet
2 participants