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

client/asset/eth: Use gwei in Balance. #1078

Merged
merged 2 commits into from
Jul 20, 2021
Merged

Conversation

JoeGruffins
Copy link
Member

closes #1076

Comment on lines +81 to +83
"istanbulBlock": 0,
"muirGlacierBlock": 0,
"berlinBlock": 0,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should put us at the same.. fork as mainnet.

Comment on lines -186 to +189
"${NODES_ROOT}/harness-ctl/mine-beta" "1"
"${NODES_ROOT}/harness-ctl/mine-alpha" "1"
"${NODES_ROOT}/harness-ctl/mine-beta" "5"
"${NODES_ROOT}/harness-ctl/mine-beta" "15"
Copy link
Member Author

Choose a reason for hiding this comment

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

I had a few times where it looked like mining on alpha made transaction inclusion speed worse. Will attempt to solve the problem in a separate pr.

Copy link
Member

Choose a reason for hiding this comment

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

Dang what a drag. Linking back to the original discussion and karalabe's comment in a different thread.

Copy link
Member

Choose a reason for hiding this comment

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

Another related idea was to have the non-miners (clients) connected to each other or at least another non-miner (or not the primary miner): ethereum/go-ethereum#2822 (comment)

ivooz: The problem goes away when all the non-mining nodes are connected to each other, previously they only knew about miner nodes.

karalabe: ... your first topology were that all nodes were connected only to the miners. This can be problematic as the miners are heavily loaded churning out blocks, so if everyone is bugging them to distribute the data too, I can imagine that they are simply overwhelmed (you essentially DDOS the miners).

I'd recommend enabling discovery (with a custom bootnode, maybe one of the miners, should be fine) so nodes can properly connect to each other and form a decent overlay network to propagate the data.

Copy link
Member

@chappjc chappjc Jul 15, 2021

Choose a reason for hiding this comment

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

Oh and about the first idea of starting 2 miners, one guy said he "updated the file "static-nodes.json" accordingly". Know what we should do there? Specify enodes? Sounds like an alt was karalabe's other suggestion of enabling discovery with a custom bootnode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will look into this directly after I get #1019 up to speed.

Copy link
Member Author

@JoeGruffins JoeGruffins Jul 16, 2021

Choose a reason for hiding this comment

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

Yeah, still scratching my head. These convos are from 2016, so maybe something different. Also, putting sleeps in the wrong places in the harness can just make everything fail. Which seems wierd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure I found and fixed the problem in #1130 Calling miner.start() miner.stop() in quick succession seems to mess with the ability of txs to make it into blocks.

@JoeGruffins JoeGruffins force-pushed the gweibalances branch 2 times, most recently from 744f5ff to 32a85b2 Compare July 15, 2021 07:51
@JoeGruffins
Copy link
Member Author

It is 18 billion eth possible correct?

@JoeGruffins JoeGruffins marked this pull request as ready for review July 15, 2021 07:55
The basic unit of eth is so small that only 18 eth worth of wei can be
stored in a uint64. Deal with eth as gwei so that we are able to ensure
that as much as 18 billion eth can be represented.
@chappjc
Copy link
Member

chappjc commented Jul 15, 2021

It is 18 billion eth possible correct?

You mean the ultimate supply? I thought it was uncapped, but with 18 million ETH per year, so with about 117 million currently existing, I'd say even 1 billion is a ton

@JoeGruffins
Copy link
Member Author

Oh no, I meant the amount of eth we can save in a uint64 as gwei. There is no eth max supply, and even if there were, they don't have a great track record of doing what that say. But 18 billion is still a far way off.

@chappjc
Copy link
Member

chappjc commented Jul 16, 2021

Hah, ok! Yup math.MaxUint64 / 1e9 is 18,446,744,073.

Copy link
Member

@chappjc chappjc 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. Didn't like having gweiFactorBig as a package-level var?

@JoeGruffins
Copy link
Member Author

Didn't like having gweiFactorBig as a package-level var?

Because it's mutatable, didn't think it was a great idea to export. Maybe a bad idea to have it as a package-level var in the first place?

The big math isn't the easiest to reason about imo. It all takes a pointer, then it returns the value? Is it returning the same value, did it mutate the original value? Need to check the documentation every time to refresh my memory. Just seems safer to do that math locally every time, with a new *big.Int, so possible future mistakes don't mess up everything else.

What do you think?

@chappjc
Copy link
Member

chappjc commented Jul 20, 2021

Sounds good to me. Just curious. I agree about the math/big API.

@chappjc chappjc merged commit d0e5f31 into decred:master Jul 20, 2021
@chappjc chappjc mentioned this pull request Jul 20, 2021
@chappjc chappjc added the ETH label Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eth: uint64 too small to store balances
2 participants