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

cmd/faucet: fix nonce-gap problem #22145

Merged
merged 9 commits into from
Jan 8, 2021
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jan 7, 2021

The faucet sometimes croaks. When a client connects to the faucet, quite a lot of data is shipped.

faucet

Every 15s or so, ~56Kb of request-data is sent, and 1.5Kb block data is sent. In the example here, half a megabyte in 2.5 minutes.

And this is for every individual user who has the page open in a tab -- so 100 concurrent users means the server has a lot of work to do. This PR reuses the encoded json messages that goes out to the clients, to make things a little bit speedier.

Caveat: I haven't tested it...

Copy link

@DGKSK8LIFE DGKSK8LIFE left a comment

Choose a reason for hiding this comment

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

Yeah I think you should test it before making the PR lol

@holiman
Copy link
Contributor Author

holiman commented Jan 8, 2021

This now also fixes two other problems:

  • The faucet incorrectly removed items from the start of the list of transactions, by checking nonces. However, the head of the list are the newest transactions, so the list just keeps growing (unless everything goes through)
  • The faucet now avoids updating the list of transactions on every new block, reducing the overall data traffic quite a lot EDIT: reverted this, it would need a UI update aswell

@holiman
Copy link
Contributor Author

holiman commented Jan 8, 2021

This also fixes a more suble flaw

It was ordered like this: [tx16,tx15,tx14]. And it erroneously tried to delete from the left, so if nonce now is 15, because tx14 was included, it would fail to delete, and leave all three there [tx16,tx15,tx14] instead of [tx16,tx15]. Later on, when it signs the next one:

            tx := types.NewTransaction(f.nonce+uint64(len(f.reqs)), address, amount, 21000, f.price, nil)

The next nonce would be 15 + 3= 18, instead of 15+2=17, causing a nonce-gap. This doesn't surface unless a few txs build up.
Looking at the live rinkeby faucet tx feed, there's a nonce-gap there:

clip | jq ".requests | .[] | .tx | .nonce" | tail
"0x60a31"
"0x60a30"
"0x60a2f"
"0x60a2e"
"0x60a2d"
"0x60a2c"
"0x60a2a"
"0x60a29"
"0x60a27"
"0x60a26"

@holiman
Copy link
Contributor Author

holiman commented Jan 8, 2021

This was borked by #22018

@holiman holiman changed the title cmd/faucet: avoid encoding for each client cmd/faucet: fix nonce-gap, reduce traffic amount Jan 8, 2021
cmd/faucet/faucet.go Outdated Show resolved Hide resolved
cmd/faucet/faucet.go Outdated Show resolved Hide resolved
cmd/faucet/faucet.go Outdated Show resolved Hide resolved
for len(f.reqs) > 0 && f.reqs[0].Tx.Nonce() < f.nonce {
f.reqs = f.reqs[1:]
// The requests are ordered as [txN, txN-1, .. txN-M]
for i := len(f.reqs) - 1; i > 0; i-- {
Copy link
Member

Choose a reason for hiding this comment

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

for i := len(f.reqs) - 1; i >= 0; i-- {


data, _ := json.Marshal(map[string]interface{}{
"funds": new(big.Int).Div(f.balance, ether),
"funded": f.nonce,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the f.nonce, f.reqs, etc internal states need the lock protection?

@karalabe
Copy link
Member

karalabe commented Jan 8, 2021

Well, the ampount of data currently sent i because the faucet is stuck and txs are piling up. If there was no bug, the amount of data would be insignificant too.

@holiman
Copy link
Contributor Author

holiman commented Jan 8, 2021

Deployed on https://faucet.rinkeby.io/

if err != nil {
return err
}
_, err = w.Write(data)
Copy link
Member

Choose a reason for hiding this comment

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

The original WiteJSON also closed the writer

// WriteJSON writes the JSON encoding of v as a message.
//
// See the documentation for encoding/json Marshal for details about the
// conversion of Go values to JSON.
func (c *Conn) WriteJSON(v interface{}) error {
	w, err := c.NextWriter(TextMessage)
	if err != nil {
		return err
	}
	err1 := json.NewEncoder(w).Encode(v)
	err2 := w.Close()
	if err1 != nil {
		return err1
	}
	return err2
}

@holiman holiman changed the title cmd/faucet: fix nonce-gap, reduce traffic amount cmd/faucet: fix nonce-gap problem Jan 8, 2021
@holiman
Copy link
Contributor Author

holiman commented Jan 8, 2021

Redeployed, works now again

@karalabe karalabe added this to the 1.10.0 milestone Jan 8, 2021
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants