Conversation
rmulhol
left a comment
There was a problem hiding this comment.
LGTM!
Was initially planning to approve but want to raise two things worth considering before we merge:
- if we apply this to a live running instance, it's likely that the same node will be treated as different (creating a new row) on startup - since it'll have a new
ID - we lose the ability to distinguish between multiple nodes running on the same
ipcPath(e.g. if you sync against a local parity node, destroy that node + sync a new one and restart - then those nodes will be treated as the same). I don't think this is realistically a problem but worth thinking about
| ) | ||
|
|
||
| var EmpytHeaderHash = "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347" | ||
| var EmptyHeaderHash = "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347" |
rmulhol
left a comment
There was a problem hiding this comment.
Still welcome others' thoughts but thinking that the issues raised above shouldn't be blockers (and perhaps warrant getting this merged sooner rather than later)
|
Those issues you mentioned @rmulhol are good thoughts to consider. Seems like if we do decide to switch over to geth0 (the non-diffing node) then the first issue won't be much of an issue if we get this merged in before the switch over, since it's a new node anyway, right? And then the other thing I'm thinking is the note that @gslaughl mentioned above about needing to make sure that we change the node's unique identity to a combo of network + ipcPath before we merge this in and switch over to geth0, right? |
3cd68b6 to
18d2ae7
Compare
| `INSERT INTO eth_nodes (genesis_block, network_id, eth_node_id, client_name) | ||
| VALUES ($1, $2, $3, $4) | ||
| ON CONFLICT (genesis_block, network_id, eth_node_id) | ||
| DO UPDATE |
There was a problem hiding this comment.
Trying to understand the implications of this - but how come we're changing what we're updating on conflict? Also, kind of wondering if we want to do anything on conflict. 🤔
There was a problem hiding this comment.
Yeah sorry, this bit is confusing. You're totally right, we don't want to do anything on conflict-- and this 'update' will never really do anything, since the conflict will only occur when the genesis_blocks are already equal.
Unfortunately, Postgres won't return anything from a RETURNING statement (line 53) after an ON CONFLICT DO NOTHING. So this is just an arbitrary, pointless update that's only there to get line 53 to work. I went ahead and reduced the update down to just 1 column (genesis_block), since I didn't see the point of faux-updating more than 1 column.
There was a problem hiding this comment.
Thinking I'll add a comment like "pointless update to make RETURNING work" or something
There was a problem hiding this comment.
Can we do a get or create? Similar to: https://github.com/makerdao/vulcanizedb/blob/staging/libraries/shared/repository/address_repository.go#L39
There was a problem hiding this comment.
Yeah good call. That version seems a little more readable
18d2ae7 to
34fdbe9
Compare
Use IPC path as node ID
34fdbe9 to
44123c7
Compare
This is a 2-parter. The first commit uses the result of
web3_clientVersioninstead of thenamefromadmin_nodeInfo, and the other (more optional) commit uses theipcPathin place of the nodeid. Both of these changes are an attempt to get all the info we need without relying on theadmininterface.The idea is that I'd expect nodes to be unique by a combination of their
ipcPathandclientVersion, but I don't know for sure whether that's the case. Also, if we decide that this is a good way to move forward, then we'll probably need to change some logic somewhere so that nodes are unique by a combination of those 2 fields, rather than justid.