Replace watchdogs with ping #1491

gladcow · 2017-06-07T21:52:17Z

Rationale: Watchdogs have proved quite problematic. We should be able to replace them by integrating sentinel information into Masternode ping objects. Need new rpc call for sentinel to update its timestamp and version then add these to masternode ping's.

This requires changes to both dashcore and sentinel (sentinel project has PR#17 with the same name). The changes are made in a backward compatible way so that dashcore and sentinel do not necessarily have to be upgraded at the same time.

Changes for dashcore:
add rpc call allowing sentinel to ping node and set sentinel version (sentinelping);
add field to governanceinfo indicating whether sentinel ping is enabled or not;
add sentinel ping flag field to CMasternodePing, this flag is set to 1 when last sentinel ping was less then MASTERNODE_WATCHDOG_MAX_SECONDS, and is set to 0 in other case;
add sentinel version field to CMasternodePing;
set CMasternode::nTimeLastWatchdogVote to current time during sentinel ping;
bump protocol version;

UdjinM6 · 2017-06-08T02:11:20Z

Thanks! However we do not merge to master. Changed base branch to v0.12.2.x for you now, pls specify it correctly next time.

Needs rebase.

UdjinM6

See inline comments
+

You need to distinguish between different formats of mnp in CMasternodeMan::ProcessMessage and deserialize accordingly otherwise you'll get lots of exceptions like " Exception 'CDataStream::read(): end of data: unspecified iostream_category error' caught, normally caused by a message being shorter than its stated length".
Bumping CMasternodeMan::SERIALIZATION_VERSION_STRING might also be a good idea.

UdjinM6 · 2017-06-08T12:35:08Z

src/activemasternode.cpp

There is some mess in the message here

UdjinM6 · 2017-06-08T12:38:49Z

src/activemasternode.h

Needs empty line above public

UdjinM6 · 2017-06-08T12:40:28Z

src/instantx.h

I don't think you should bump MIN_... consts, what is the rationale? (same for similar changes below)

UdjinM6 · 2017-06-08T12:44:03Z

src/masternode.h

If it's just a flag - why not bool? Also it's quite inconsistent to have variables with the same exact name but with different types (talking about this and activemasternode.* here).

UdjinM6 · 2017-06-08T12:45:08Z

src/masternode.h

Since versions are 1,2,3 etc do we really need 4 bytes here?

UdjinM6 · 2017-06-08T12:59:16Z

src/masternodeman.cpp

I doubt this solves anything - SetMasternodeLastPing is only used by CActiveMasternode (the local one itself) and we should take sentinelPing into account for every other masternode as well. Should fix corresponding conditions or otherwise all other masternodes are going to be in WATCHDOG_EXPIRED state.

Yes, this should be called from CMasternodeMan::ProcessMessage or in some function called from there.

UdjinM6 · 2017-06-08T13:01:28Z

src/rpcgovernance.cpp

I doubt this belongs here, getgovernanceinfo's purpose is to print out global governance constants, not the state of the current masternode.

Why not ?

"getgovernanceinfo's purpose is to print out global governance constants, not the state of the current masternode." - I don't know that it's purpose should be that narrowly construed. It contains lastsuperblock and nextsuperblock which are not global constants.

Also hassentinelping isn't really about the state of the masternode but rather is a version dependent property.

ok, I wasn't quite accurate about constants, let's read that as governance globals instead. In any case, I think that node specific field like that doesn't belong here. It's also excessive since you can simply get protocolversion via getinfo.

Yes, really excessive, I will remove it.

tgflynn · 2017-06-14T00:56:22Z

A couple of comments:

I see commits in the PR which have already been merged, so I think your branch needs to be rebased.
We need to do something with sentinelVersion. The purpose of adding it is so we can see what version of sentinel masternodes are running. Should be added to the masternode list rpc command output.

I think it should probably be added to the output of the simple "masternode list" command, not just "masternode list full" because the later is very computationally inefficient. Since it doesn't really make sense to show the sentinel version without the protocol version, that should probably be added to "masternode list" as well.

UdjinM6 · 2017-06-14T01:34:49Z

I think it should probably be added to the output of the simple "masternode list" command, not just "masternode list full" because the later is very computationally inefficient. Since it doesn't really make sense to show the sentinel version without the protocol version, that should probably be added to "masternode list" as well.

Every output other then "masternode list full" is for a single thing to query, so I would not change this and would keep the default output as is. If we want sentinel version to be displayed in the list it could go to "full" + a new one like "sentinel" or smht like that.

tgflynn · 2017-06-14T01:48:01Z

Maybe we could add a command: "masternode list info" which would display all the mn information that can be queried efficiently (including sentinelVersion).

UdjinM6 · 2017-06-14T01:57:21Z

Maybe we could add a command: "masternode list info" which would display all the mn information that can be queried efficiently (including sentinelVersion).

You mean like some "static" info? The inefficiency in full comes mostly from UpdateLastPaid() (which results are indeed "dynamic") iirc, so if we drop lastpaidtime and lastpaidblock in that new info command that would do it. But imo introducing info should be done in a separate PR.

tgflynn · 2017-06-14T02:03:17Z

Another option would be to fix "masternode list full" by keeping the last paid info up to date instead of recomputing it on request. But that would be a much bigger job and definitely a separate PR.

UdjinM6 · 2017-06-14T02:06:16Z

Another option would be to fix "masternode list full" by keeping the last paid info up to date instead of recomputing it on request.

It's already the case for MNs. It's not the case for all other nodes because the first computation takes time and most normal nodes/qt clients will never use it. Second request should be fast even on non-MNs already.

tgflynn

Found a couple of small issues and bigger problem with the rpc command.

tgflynn · 2017-06-14T11:55:30Z

src/activemasternode.cpp


+bool CActiveMasternode::UpdateSentinelPing(int version)
+{
+    if(!mnodeman.Has(vin)) {


I don't think there's any real need for this block of code since you're just setting some variables. There's nothing to protect here.

tgflynn · 2017-06-14T11:56:37Z

src/rpcmasternode.cpp

+        );
+    }
+
+    if(activeMasternode.nState == ACTIVE_MASTERNODE_INITIAL ||


I don't see a need for this conditional, you can just call UpdateSentinelPing directly.

tgflynn · 2017-06-14T12:06:08Z

src/rpcmasternode.cpp

+        return false;
+
+
+    activeMasternode.UpdateSentinelPing(params[1].get_int());


I don't this will work unless you add:

{ "sentinelping", 0}

to the vRPCConvertParams[] array in rpcclient.cpp.

UdjinM6

see inline comments

As a general note, it feels like this is not going to be as easy as I initially thought because these changes are not backward compatible on p2p level actually. We should probably release a mandatory pre-12.2 update for 12.1 at some point to let old nodes recognize new message format/signatures (that is probably inevitable anyway). This should make migration smoother (or otherwise network will split because old nodes will ban new ones for incorrect messages/signatures).

UdjinM6 · 2017-06-15T11:53:23Z

src/masternode.cpp

 {
    LOCK(cs);
-    nTimeLastWatchdogVote = GetTime();
+    if(t == 0)


This can be written in one line:

nTimeLastWatchdogVote = (t == 0) ? GetTime() : t;

Also I would prefer for an argument to be named in a more descriptive manner e.g. nVoteTime or smth like that

UdjinM6 · 2017-06-15T12:29:55Z

src/activemasternode.h

    bool SendMasternodePing();

+    //  sentinel ping data
+    int64_t sentinelPing;


consider using current naming convention i.e. nSmth for numbers (including Time could also be a good idea to make it a bit more descriptive)

UdjinM6 · 2017-06-15T12:31:50Z

src/masternode.h

    int64_t sigTime; //mnb message times
    std::vector<unsigned char> vchSig;
+    bool sentinelIsActual; // true if last sentinel ping was actual
+    uint16_t sentinelVersion;


use current naming convention here too

UdjinM6 · 2017-06-15T12:36:02Z

src/masternode.h

+        if(ser_action.ForRead() && (s.size() == 0))
+            return;
+        READWRITE(sentinelIsActual);
+        READWRITE(sentinelVersion);


We should probably also include sentinelIsActual and sentinelVersion into mnp signature (see Sign/CheckSignature functions), otherwise nodes in-between will be able to change/fake it. this can be a problem because some nodes won't update wd time and will consider sentinel on original node to be non-active and this could result in network split. Note that functions I mentioned above should be able to create/check both versions of signatures depending on (original, for verification) MN protocol version.

Ignore this for now

UdjinM6

There are few places where new VersionInfo should be used to preserve consistency imo. Also I'm not quite sure if it has to be defined in utils because it's used specifically for sentinel, but it's probably ok.

UdjinM6 · 2017-06-27T20:34:11Z

src/activemasternode.cpp

    return true;
 }

+bool CActiveMasternode::UpdateSentinelPing(int version)


should probably use VersionInfo here

I've thought that VersionInfo is convertor, not storage for version information. But if you think it is better to use it to store the version itself, I change this variables type. This is also require some changes in VersionInfo class itself (i.e. default initialization, serialization, etc.), in this case it is better to move it from utils.h header, I think. May be it is better to rename this class to VersionInfoConverter, for example?

UdjinM6 · 2017-06-27T20:34:24Z

src/activemasternode.h


+    //  sentinel ping data
+    int64_t nSentinelPingTime;
+    uint32_t nSentinelVersion;


should probably use VersionInfo here

UdjinM6 · 2017-06-27T20:34:33Z

src/activemasternode.h

    std::string GetStatus() const;
    std::string GetTypeString() const;

+    bool UpdateSentinelPing(int version);


should probably use VersionInfo here

UdjinM6 · 2017-06-27T20:34:48Z

src/masternode.h

    int64_t sigTime; //mnb message times
    std::vector<unsigned char> vchSig;
+    bool fSentinelIsCurrent; // true if last sentinel ping was actual
+    uint32_t nSentinelVersion; // MSB is always 0, other 3 bits corresponds to x.x.x version scheme


should probably use VersionInfo here

tgflynn · 2017-06-27T22:02:44Z

src/util.cpp

+    {
+        if(tokens[idx].length() == 0)
+            throw std::runtime_error("Invalid version format");
+        if(!std::none_of(tokens[idx].begin(), tokens[idx].end(),


Looks like this will allow non-digit characters in the version string. Is this intentional ?

UdjinM6 · 2017-06-28T12:41:02Z

Looks good 👍

tgflynn · 2017-06-28T14:12:18Z

utACK

UdjinM6 · 2017-07-04T14:53:01Z

needs rebase

- fix the error message text in CActivbeMasternodeUpdateSentinelPing; - add empty string before public: in CActiveMasternode class declaration; - rename field sentinelPing in CMasternodePing to sentinelIsActual and change $ - decrease sentinelVersion field size to uint16_t;

- call UpdateWatchdogVoteTime in CMasternodeMan::ProcessMessage - deserialize masternodeping from the previous version archive without exception - add ability to set time in UpdateWatchdogVoteTime - set nTimeLastWatchdogVote to masternode ping sigTime if sentinel is actual - bump CMasternodeMan::SERIALIZATION_VERSION_STRING

Version format is "x.x.x"

UdjinM6

Looks good 👍

utACK

4af241b320 Merge bitcoin-core/secp256k1#1535: build: Replace hardcoded "auto" value with default one f473c959f0 Merge bitcoin-core/secp256k1#1543: cmake: Do not modify build types when integrating by downstream project d403eea484 Merge bitcoin-core/secp256k1#1546: cmake: Rename `SECP256K1_LATE_CFLAGS` and switch to Bitcoin Core's approach d7ae25ce6f Merge bitcoin-core/secp256k1#1550: fix: typos in secp256k1.c 0e2fadb20c fix: typos in secp256k1.c 69b2192ad4 Merge bitcoin-core/secp256k1#1545: cmake: Do not set `CTEST_TEST_TARGET_ALIAS` 5dd637f3cf Merge bitcoin-core/secp256k1#1548: README: mention ellswift module 7454a53736 README: mention ellswift module 4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS` f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS` 158f9e5eae cmake: Do not modify build types when integrating by downstream project 35c0fdc86b Merge bitcoin-core/secp256k1#1529: cmake: Fix cache issue when integrating by downstream project 4392f0f717 Merge bitcoin-core/secp256k1#1533: tests: refactor: tidy up util functions (dashpay#1491) bedffd53d8 Merge bitcoin-core/secp256k1#1488: ci: Add native macOS arm64 job 4b8d5eeacf Merge bitcoin-core/secp256k1#1532: cmake: Disable eager MSan in ctime_tests f55703ba49 autotools: Delete unneeded compiler test 396e885886 autotools: Align MSan checking code with CMake's implementation abde59f52d cmake: Report more compiler details in summary 7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory` 4d9645bee0 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option a06805ee74 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option 1791f6fce4 Merge bitcoin-core/secp256k1#1517: autotools: Disable eager MSan in ctime_tests 26b94ee92a autotools: Remove "auto" value of `--with-ecmult-gen-kb` option 122dbaeb37 autotools: Remove "auto" value of `--with-ecmult-window` option e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions 0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions 0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h 0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude` 59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...` ebfb82ee2f ci: Add job with -fsanitize-memory-param-retval e1bef0961c configure: Move "experimental" warning to bottom 55e5d975db autotools: Disable eager MSan in ctime_tests ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable 218f0cc93b ci: Add native macOS arm64 job git-subtree-dir: src/secp256k1 git-subtree-split: 4af241b32099067464e015fa66daac5096206dea

UdjinM6 changed the base branch from master to v0.12.2.x June 8, 2017 01:58

UdjinM6 added this to the 12.2 milestone Jun 8, 2017

gladcow force-pushed the watchdog_to_ping branch from 4ac3732 to 2628335 Compare June 8, 2017 10:17

gladcow changed the title ~~Replace watchdogs with ping~~ [WIP] Replace watchdogs with ping Jun 8, 2017

gladcow force-pushed the watchdog_to_ping branch from 2628335 to 2def10f Compare June 8, 2017 11:30

gladcow changed the title ~~[WIP] Replace watchdogs with ping~~ Replace watchdogs with ping Jun 8, 2017

UdjinM6 requested changes Jun 8, 2017

View reviewed changes

gladcow force-pushed the watchdog_to_ping branch from 542d76a to ca478c5 Compare June 14, 2017 10:35

tgflynn suggested changes Jun 14, 2017

View reviewed changes

UdjinM6 reviewed Jun 15, 2017

View reviewed changes

UdjinM6 reviewed Jun 27, 2017

View reviewed changes

tgflynn reviewed Jun 27, 2017

View reviewed changes

UdjinM6 approved these changes Jun 28, 2017

View reviewed changes

Sergey added 7 commits July 4, 2017 08:16

Add hassentinelping to governanceinfo

4527112

sentinelping rpc call

92cecb0

additional fields in mnp

6d7d99c

sentinel ping implementation

a2ea036

change sentinel state to byte in mnp

68b330c

use adjusted time in sentinel ping

a6a6d9f

update nTimeLastWatchdogVote if sentinel ping is actual

2af08c9

Sergey added 12 commits July 4, 2017 08:16

remove unused fields

6069477

bump protocol to 70207

cb82e7f

revert proto bump for MIN_... consts

e536054

revert changes in getgovernanceinfo

e833ee2

remove mn state checks and add correct rpc param convertion

a7adcd6

fix var names

00c3c3b

Helper class for version in string and integer form

16c79ee

String version in sentinel ping

3d0d5c2

Version format is "x.x.x"

test for bacward compatibility in serialization

b853de2

Change VersionInfo class to convert functions

fe62c9f

gladcow force-pushed the watchdog_to_ping branch from 74540ca to fe62c9f Compare July 4, 2017 15:59

UdjinM6 approved these changes Jul 4, 2017

View reviewed changes

UdjinM6 merged commit a439e98 into dashpay:v0.12.2.x Jul 4, 2017

gladcow deleted the watchdog_to_ping branch July 4, 2017 18:10

		return false;


		activeMasternode.UpdateSentinelPing(params[1].get_int());

Replace watchdogs with ping #1491

Replace watchdogs with ping #1491

Uh oh!

Conversation

gladcow commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Jun 8, 2017

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgflynn commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgflynn commented Jun 14, 2017

Uh oh!

UdjinM6 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgflynn commented Jun 14, 2017

Uh oh!

UdjinM6 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgflynn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gladcow Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

gladcow commented Jun 7, 2017 •

edited

Loading

UdjinM6 Jun 8, 2017 •

edited

Loading

tgflynn commented Jun 14, 2017 •

edited

Loading

UdjinM6 commented Jun 14, 2017 •

edited

Loading

UdjinM6 commented Jun 14, 2017 •

edited

Loading

UdjinM6 commented Jun 14, 2017 •

edited

Loading

gladcow Jun 27, 2017 •

edited

Loading