Expose node internals #445
Conversation
Signed-off-by: Roman Khimov <[email protected]>
roman-khimov
requested review from
carpawell and
cthulhu-rider
as code owners
roman-khimov
force-pushed
the
expose-node-internals
branch
from
60ec224 to
c4a3bf0
Compare
| // all of the node structure to contract. | ||
| type Node2 struct { | ||
| // Addresses are to be used to connect to the node. | ||
| Addresses []string |
There was a problem hiding this comment.
There was a problem hiding this comment.
That one is just an attribute, attribute it'll be. The structure obviously follows the data we have defined for nodes.
There was a problem hiding this comment.
what usecase then for this field if it is not full and every "good" client should see attributes too?
There was a problem hiding this comment.
Are you trying to tell me that it's never filled by nodes? This structure just follows protobuf, whatever is there we have here as well without thinking much about it.
There was a problem hiding this comment.
Are you trying to tell me that it's never filled by nodes?
of course, it is used but there is also more information that is called "external addresses" in our code. anyway, ok let it be
| ctx := storage.GetContext() | ||
|
|
||
| if n.State != nodestate.Online { | ||
| panic("can't add non-online node") |
There was a problem hiding this comment.
in some of my PRs i tried to put more context in errors, however, it was reworked and not merged but still, is it a good practice to add more info to contracts panics?
There was a problem hiding this comment.
What kind of info do you want here?
There was a problem hiding this comment.
in the "can't add non-online node" log i would like to see what state i am trying to add. i may not even know where the bug is and believe that i am doing everything right. at least, this log is what almost all of us would add to the error in a pure go-code
There was a problem hiding this comment.
You don't have a lot of possibilities, really. Offline/maintenance.
There was a problem hiding this comment.
or unknown 1234. ok not that critical
| @@ -665,6 +780,32 @@ func filterNetmap(ctx storage.Context) []Node { | |||
| return result | |||
| } | |||
|
|
|||
| func fourBytesBE(num int) []byte { | |||
There was a problem hiding this comment.
this is too specific and i see the only usage of it, so suggest to leave netmapNode2AtEpochPrefix() []byte for now. But dont insist
roman-khimov
force-pushed
the
expose-node-internals
branch
from
c4a3bf0 to
f7cea37
Compare
This format is added in parallel to existing one because we can't migrate in the contract easily, it'd require complete protobuf parser in VM. Instead we provide a secondary map here that works similar to the old one (except it doesn't have any listing or duplication issues). The expection is that nodes will send _both_ transactions starting from some time, we'll monitor them and transition to using new map only in subsequent IR/SN release. Then contract could be updated to drop the old data. Signed-off-by: Roman Khimov <[email protected]>
Reuse standard code. Signed-off-by: Roman Khimov <[email protected]>
It uses alphabet multiaddress and it's well-known, no need to repeat the getter. Signed-off-by: Roman Khimov <[email protected]>
roman-khimov
force-pushed
the
expose-node-internals
branch
from
f7cea37 to
063870e
Compare
No description provided.