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.

Sign up for GitHub

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

Jump to bottom

client/eth: Fix test gas calculations. #1346

Merged
merged 5 commits into from
Jan 8, 2022
Merged

client/eth: Fix test gas calculations. #1346

merged 5 commits into from
Jan 8, 2022

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Dec 13, 2021 •
edited
Loading 

Use the base gas fee for the block before a tx was mined to caculate
exact gas used and test using that expected amount.

closes #1342

@JoeGruffins
Copy link
Member Author

I believe this does fix some random errors, but not any called out in the issues...

JoeGruffins
JoeGruffins commented Dec 13, 2021
View reviewed changes
client/asset/eth/nodeclient.go
Comment on lines +273 to 294
tx, blockHash, _, index, err := n.leth.ApiBackend.GetTransaction(ctx, txHash)
if err != nil {
return nil, nil
return nil, err
}
if tx == nil {
return nil, fmt.Errorf("transaction %v not found", txHash)
}
Copy link
Member Author

@JoeGruffins JoeGruffins Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some convo in #1327

The nil check is very helpful for debugging.

JoeGruffins
JoeGruffins commented Dec 13, 2021
View reviewed changes
client/asset/eth/nodeclient.go Outdated
@@ -513,7 +518,7 @@ func (n *nodeClient) transactionConfirmations(ctx context.Context, txHash common
// tx pool, and when our peers are ready to supply the info. I saw a
// CoinNotFoundError in TestAccount/testSendTransaction, but haven't
// reproduced.
return 0, asset.CoinNotFoundError
return 0, fmt.Errorf("cannot find %v: %w", txHash, asset.CoinNotFoundError)
Copy link
Member Author

@JoeGruffins JoeGruffins Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this randomly hit sometimes, but other nodes do see the tx. I don't know why it's being missed. Would make sense though if related to the issue with missing txs the first try in #1327

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at btc and dcr, it seems that we are always returning the asset.CoinNotFoundError unwrapped. In core, we are always using errors.Is, so this won't matter for us. But any consumer using direct comparison, if err == asset.CoinNotFoundError, would now behave differently.

martonp
martonp approved these changes Dec 21, 2021
View reviewed changes
Copy link
Collaborator

@martonp martonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, tests passed on a fresh harness.

client/asset/eth/nodeclient.go Outdated
@@ -209,6 +209,8 @@ func (n *nodeClient) balance(ctx context.Context) (*Balance, error) {

for _, tx := range pendingTxs {
from, _ := ethSigner.Sender(tx) // zero Address on error
// Intentionally ignoring receives from addresses that do not
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think those would even be listed in pending transactions... I think this is more of a sanity check.

Copy link
Member Author

@JoeGruffins JoeGruffins Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "those" receives from addresses that do not belong to the wallet? I think this will show all txs in the mempool?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From go-ethereum/light/txpool.go

// TxPool implements the transaction pool for light clients, which keeps track
// of the status of locally created transactions, ...

More accurately, ignoring transactions that are not from us, but that's really what the code says, so I don't know that the doc adds much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the doc.

@JoeGruffins
Copy link
Member Author

Oh man, after rebase tests are failing in two spots consistently:

nodeclient_harness_test.go:966: test "ok before locktime": expected isRedeemable to be true, but got false

and

nodeclient_harness_test.go:1184: wrong refunder: refund error: insufficient funds for gas * price + value

What happened. anyone else? I updated to geth 1.10.13

@martonp
Copy link
Collaborator

martonp commented Dec 23, 2021 •
edited
Loading

Looks like due to the key derivation change in c54f8d1 , the participantPrivKey is being derived differently, and the participantAddr no longer has any funds.

The new derivation gives these values:

dcrdex/client/asset/eth/nodeclient_harness_test.go

Line 80 in f333d1f

participantPrivKey = "4fc4f43c00bc6550314b8561878edbfc776884e006ad51a2fe2c054f85cfbd12"

@chappjc
Copy link
Member

chappjc commented Dec 23, 2021

Ah dang, my bad. Didn't anticipate the hard coded eth keys in the harness.

@JoeGruffins
Copy link
Member Author

Ok, just need to change the keys then. Will do. Thx @martonp

buck54321
buck54321 requested changes Dec 28, 2021
View reviewed changes
client/asset/eth/nodeclient.go Outdated
@@ -513,7 +518,7 @@ func (n *nodeClient) transactionConfirmations(ctx context.Context, txHash common
// tx pool, and when our peers are ready to supply the info. I saw a
// CoinNotFoundError in TestAccount/testSendTransaction, but haven't
// reproduced.
return 0, asset.CoinNotFoundError
return 0, fmt.Errorf("cannot find %v: %w", txHash, asset.CoinNotFoundError)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at btc and dcr, it seems that we are always returning the asset.CoinNotFoundError unwrapped. In core, we are always using errors.Is, so this won't matter for us. But any consumer using direct comparison, if err == asset.CoinNotFoundError, would now behave differently.

client/asset/eth/nodeclient_harness_test.go Outdated
@@ -682,8 +689,12 @@ func testInitiate(t *testing.T) {
t.Fatalf("%s: balance error: %v", test.name, err)
}

gasPrice, err := feesAtBlk(ctx, ethClient, receipt.BlockNumber.Int64()-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the transaction was not being included in the block we expected?

Copy link
Member Author

@JoeGruffins JoeGruffins Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it just means that the base fee for a block is encoded in (or calculated from) the previous block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's in the header, the latest commit makes more sense for getting the base fee from mined blocks.

@JoeGruffins
Copy link
Member Author

Is there a issue for this error?

https://github.com/decred/dcrdex/runs/4654985696?check_suite_focus=true

@chappjc
Copy link
Member

chappjc commented Dec 29, 2021

Is there a issue for this error?

https://github.com/decred/dcrdex/runs/4654985696?check_suite_focus=true

looks new

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Dec 29, 2021 •
edited
Loading

looks new

Is it #1268 ? Looks very similar.

@JoeGruffins
Copy link
Member Author

Just rebased.

@JoeGruffins
Copy link
Member Author

Need to rebase again.

@chappjc
Copy link
Member

chappjc commented Jan 7, 2022

Yeah, sorry @JoeGruffins. This PR is g2g, but I merged @martonp's token nodeclient PR first.

@JoeGruffins
client/eth: Fix test gas calculations.
e16c5af 
Use the base gas fee for the block before a tx was mined to caculate
exact gas used and test using that expected amount.
@JoeGruffins
Remove unneeded doc.
8a3c1b1
@JoeGruffins
Correct simnet participant address.
03fbc2f
@JoeGruffins
Return asset.CoinNotFoundError.
92a3344
@JoeGruffins
Use header base fee.
2aaf6a8
@chappjc chappjc merged commit e05191a into decred:master Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chappjc chappjc chappjc left review comments

@buck54321 buck54321 buck54321 approved these changes

@martonp martonp martonp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

eth/client/simnet_tests: testSendSignedTransaction failing.
4 participants
@JoeGruffins @martonp @chappjc @buck54321