Geth v1.12.2 nits #1099
Geth v1.12.2 nits #1099
Conversation
| // dataLock protects the [data] field to prevent a race condition | ||
| // in the transaction pool tests. TODO remove after re-implementing | ||
| // tx pool to be synchronous. | ||
| dataLock sync.RWMutex |
There was a problem hiding this comment.
Are we sure this is safe to remove?
There was a problem hiding this comment.
I think these were fixed way before with ava-labs/coreth@8e32d43#diff-64a06d0cea30e12d6001fb322e583930c1cf3879bca4e8019fa89499ed9f7de4
Also seems like this was only an issue in tests. cc @aaronbuchwald
There was a problem hiding this comment.
I also added -race flag to UTs to catch these.
cmd/utils/flags.go
Outdated
| if len(set) > 1 { | ||
| Fatalf("Flags %v can't be used at the same time", strings.Join(set, ", ")) | ||
| } | ||
| } |
There was a problem hiding this comment.
I added this so we can avoid copying the entire cmd/utils package when doing the geth update.
This is because the entire cmd/utils package imports a lot of geth code.
So I think it's better to leave this in place with only the 2 functions we use from this package.
There was a problem hiding this comment.
I totally forgot we refactored this. Reverted it and moved Fatalf function to cmd.go as in upstream.
| // network upgrades | ||
| func (c *ChainConfig) AvalancheRules(blockNum *big.Int, timestamp uint64) Rules { | ||
| func (c *ChainConfig) Rules(blockNum *big.Int, timestamp uint64) Rules { |
There was a problem hiding this comment.
if we rename this method to Rules, like it is in upstream, should we also take the same bool isMerge parameter?
Honestly it's kinda weird that they have the isMerge parameter, but I think if the name matches the signature should also match, otherwise perhaps it's better to leave it as AvalancheRules.
There was a problem hiding this comment.
IMO we should not use that isMerge param. but I also slightly think it is probably better to use same Rules function. I can revert that if you think we should be using AvalancheRules instead.
Mostly cosmetic changes to reduce diff between upstream