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

1363 reason field deregistrations #1369

Merged
merged 20 commits into from Mar 29, 2021

Conversation

darcys22
Copy link
Collaborator

@darcys22 darcys22 commented Dec 7, 2020

Addressing #1363

Adds a decommission reason to the service node decommission votes.

@darcys22 darcys22 force-pushed the 1363-reason-field-deregistrations branch from ed5f5d9 to 6311332 Compare January 19, 2021 21:46
@darcys22 darcys22 marked this pull request as ready for review January 20, 2021 03:23
@darcys22
Copy link
Collaborator Author

This should be ready for review, there will be additional reasons added after #1370 is merged.

Only query I had was regarding the hashing of the obligation dereg transaction. The code had previously backed out the state integer if it was a deregister. Should this remain as is after implementing the reason field:

  static crypto::hash make_state_change_vote_hash(uint64_t block_height, uint32_t service_node_index, new_state state)
  {
    uint16_t state_int = static_cast<uint16_t>(state);

    auto buf = tools::memcpy_le(block_height, service_node_index, state_int);

    auto size = buf.size();
    if (state == new_state::deregister)
        size -= sizeof(state_int); // Don't include state value for deregs (to be backwards compatible with pre-v12 dereg  votes)

    crypto::hash result;
    crypto::cn_fast_hash(buf.data(), size, result);
    return result;
  }

@jagerman
Copy link
Member

Should this remain as is after implementing the reason field:

Yeah. The state change vote hash is meant to unique identify the (height, SN index, state) tuple. But prior to v12 there was no "state" parameter (we only had deregs) so if it's a dereg we just use the hash of (height, SN index). It's not necessary anymore, but if we change it now then the hash won't match up across different versions, so easier just to leave it.

@Azazel020
Copy link

The node on this server was expired so I updated the server to the latest version 8.1.5 and started the node . After some time the storage server crashed and the server went in decom. I received the notification from the bot, restarted the VPS and all services were up again. At that point I had enough blocks uptime credits to get reinstated by the network but ended in a decom. It seems that with the new version/qty of nodes the credit time with a new start (60+ blocks) isn`t enough to fix things.
Attached both log files.

log_oxen_node.txt
log_oxen_storage_server.txt

@darcys22
Copy link
Collaborator Author

Hi azazel, this thread is regarding an unimplemented feature and not the right place to post a bug report. However from those logs it appears that the storage server was not communicating with the network which would have caused the decom even if the node was submitting uptime proofs

@darcys22 darcys22 force-pushed the 1363-reason-field-deregistrations branch from 9abdec0 to efda420 Compare March 2, 2021 00:14
@darcys22 darcys22 force-pushed the 1363-reason-field-deregistrations branch from efda420 to 8d42a3a Compare March 9, 2021 05:25
@darcys22
Copy link
Collaborator Author

darcys22 commented Mar 9, 2021

This should be ready to review now, updated to include the new timesync decommission reasons

src/wallet/wallet_rpc_server.cpp Outdated Show resolved Hide resolved
src/cryptonote_basic/tx_extra.h Outdated Show resolved Hide resolved
src/cryptonote_core/service_node_quorum_cop.cpp Outdated Show resolved Hide resolved
@darcys22 darcys22 force-pushed the 1363-reason-field-deregistrations branch 2 times, most recently from a49998b to b4ef812 Compare March 25, 2021 00:22
@jagerman
Copy link
Member

One suggestion from a community member which would be pretty nice to have:

Viewable on explorer is fine, maybe the node itself should be told, and it can report it under oxend status?

So instead of just showing "decommissioned" we could show something like

"decommissioned(storage)" or "decommissioned(uptimes,timesync,checkpoints)" or whatever our different reason flags are.

@darcys22
Copy link
Collaborator Author

Updated to print reason

@jagerman jagerman force-pushed the 1363-reason-field-deregistrations branch from c7aa27d to b5938bf Compare March 26, 2021 02:45
darcys22 and others added 10 commits March 26, 2021 14:05
Governance reward calculations were hard-coded for == HF17 rather than
>= 17, so for HF18 it was falling back to the old "add up all the
values" method that we used to use.  Updated it to support HF18, and add
a static_assert that will fail to compile (without a fix) when we add
HF19.

Also some minor cleanups (mostly indent changes for unnecessary blocks
-- ignore whitespace when looking at the diff).
darcys22 and others added 6 commits March 26, 2021 15:25
This reinterprets the leading "state" as a version field, if >= 4, and
otherwise keeps it as the state value if < 4.

It is done in such a way as to remain the same round-trip (i.e. if we
deserialize it and then reserialize) so as to not break existing
signature verification.

This lets us properly serialize/deserialize both old, reasonless state
change txes *and* new state change txes with a reason field.  Without
this syncing failed because we'd hit a state change tx and couldn't
parse it properly.  (But we also can't just "upgrade" to the new version
because that would change the serialized value and break signatures).
The state_change constructor changed (to include a version + reasons),
this updates the test suite to match.
@jagerman jagerman force-pushed the 1363-reason-field-deregistrations branch from b5938bf to d75fa1e Compare March 26, 2021 18:25
Fixes broadcast reasons being set to 0.
The RPC was returning readable strings instead of coded strings.

Also shorten the returned codes because they were a bit lengthy, and
document them in the RPC comment.
@jagerman jagerman merged commit d524d56 into oxen-io:dev Mar 29, 2021
@darcys22 darcys22 deleted the 1363-reason-field-deregistrations branch April 27, 2021 22:47
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.

None yet

4 participants