Implement Governance into the ZMQ messages #2160

PastaPastaPasta · 2018-07-01T00:21:05Z

I think I figured this one out and am done to my knowledge, but I am sure there are things I missed. This PR was inspired by @jhodges10, the lead of DashNexus (treasury funded replacement of dashcentral), possibly for use in nexus. I'd be quite surprised if I didn't miss something so just let me know and I'll try and fix it.

I accidentally added a minor formatting change, that wasn't an independent commit(other ones I reverted), in zmqnotificationinterface.cpp L:38-50ish in addition to adding the two new lines needed to make this PR work. Tell me if that's a problem.

Outside of that, this PR adds the sending of CGovernanceObject hashs over zmq when a new CGovernanceObject is added and adds the sending of CGovernanceVote hashs over zmq when a new CGovernanceVote is added. I could potentially add a pushing of a raw form, similar to transactions but I am not sure about that.

It does compile w/o problem, but I have not tried it in action yet. Will do so when I can.

Thoughts?

it seems that zmq is currently not in a working state, need to figure that out. Need to: plug in the new methods added possibly plug in the old methods, as it doesn't look like they are.

… over zmq now.

This reverts commit 2918ea4.

This reverts commit ca10558.

PastaPastaPasta · 2018-07-01T00:22:46Z

src/governance.cpp

    CheckOrphanVotes(govobj, exception, connman);

+    //Send notifications to scripts / zmq
+    GetMainSignals().NotifyGovernanceObject(govobj);


I think this is a good spot for it, but same as above. Don't know the codebase well enough to say for sure.

PastaPastaPasta · 2018-07-01T00:26:07Z

src/zmq/zmqpublishnotifier.cpp

 static const char *MSG_RAWTX      = "rawtx";
 static const char *MSG_RAWTXLOCK  = "rawtxlock";
+static const char *MSG_GVOTE      = "governancevote";
+static const char *MSG_GOBJECT    = "governanceobject";


change to hashgovernance*****? Thoughts?

src/zmq/zmqpublishnotifier.h

MrDefacto · 2018-07-01T00:32:04Z

To be consistent it should be rather:
hashgovernanceobject
hashgovernancevote

and this patch also lacks support for raw types:
rawgovernanceobject
rawgovernancevote

PastaPastaPasta · 2018-07-01T00:38:47Z

src/governance-object.cpp

    }

+    //Send notifications to scripts / zmq
+    GetMainSignals().NotifyGovernanceVote(vote);


there may be a better location for this. But I couldn't find one.

MrDefacto · 2018-07-01T00:39:17Z

This is one of my patch for 12.1 that adds support for this and even more (zeromq masternode announcements). After few tweaks it should also work with 12.3.

zeromq_masternode_governance.diff.zip

PastaPastaPasta · 2018-07-01T00:41:16Z

@MrDefacto it would be better if you could reference your PR or those commits, or even put it in a gist.

nmarley · 2018-07-01T00:43:37Z

contrib/zmq/zmq_sub.py

            print('- HASH BLOCK ('+sequence+') -')
            print(binascii.hexlify(body).decode("utf-8"))
        elif topic == "hashtx":
-            print ('- HASH TX ('+sequence+') -')


Please remove the space here

@nmarley there was a space originally, this commit removes that space. can revert this change if needed.

@paulied You're right, sorry for getting the direction of change confused. This one is actually small, but personally I would still prefer to not mix formatting and functional PRs.

One reason is, what if this PR doesn't end up being merged in? That space fix would have to go in somewhere else then, and results in double-work (albeit minor).

MrDefacto · 2018-07-01T00:44:06Z

@paulied It's wasn't PR. I've only talked about this in priv msgs with @UdjinM6 on forum. And because it was late phase of 12.2 release he decided to postpone it... and time passed...

nmarley · 2018-07-01T00:46:01Z

src/zmq/zmqnotificationinterface.cpp


-    factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;
-    factories["pubhashtx"] = CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>;
+    factories["pubhashblock"]  = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;


Can you revert these in this PR?

Do you know what is the clang-format recommendations for this type of alignment? (I am not sure myself.)

If you want me to revert that formatting sure I can(accidentally didn't put this in a separate commit, so i'd have to do it by hand but yeah), I do not actually know what clang says on it. Couldn't find anything on it. I just know the old way it was looked bad and was annoying to read. Was hoping you could advise.

I would prefer to remove formatting in this particular PR just to stick to keeping functional and formatting changes separate, for reasons described above. We do try to stick to the clang-format convention, but I'm not sure exactly what that is for this situation.

There's a clang-format program which reads the rules in the src/.clang-format file and shows the correct format (or maybe it changes the code to align w/it) based on those rules.

nmarley · 2018-07-01T01:44:21Z

src/zmq/zmqpublishnotifier.cpp

+    for (unsigned int i = 0; i < 32; i++)
+        data[31 - i] = hash.begin()[i];
+    return SendMessage(MSG_GOBJECT, data, 32);
+}


Needs EOL char

nmarley · 2018-07-01T01:49:41Z

src/zmq/zmqpublishnotifier.cpp

+    uint256 hash = vote.GetHash();
+    LogPrint("zmq", "zmq: Publish governancevote %s\n", hash.GetHex());
+    char data[32];
+    for (unsigned int i = 0; i < 32; i++)


I see you're using the same format as above, but wondering if there's a way to use existing serialization methods for this. Also wondering if we can move hash.begin() out of the loop and only call it once.

nmarley · 2018-07-01T01:51:11Z

src/zmq/zmqnotificationinterface.cpp

+
+void CZMQNotificationInterface::NotifyGovernanceVote(const CGovernanceVote &vote)
+{
+    for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i != notifiers.end(); )


The iterator initialization is probably ok, but would prefer if we can move to using auto instead.

The convention in our codebase is to call iterators it, so would prefer that name to just i.

as in for (auto it = notifiers.begin(); it != notifiers.end(); ) ? and then change the others to it too

Yep, this looks correct.

please view cb16cf0

nmarley · 2018-07-01T01:52:07Z

src/zmq/zmqnotificationinterface.cpp

+{
+    for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i != notifiers.end(); )
+    {
+        CZMQAbstractNotifier *notifier = *i;


This is an extra un-necessary variable. You can get the value of the iterator by dereferencing, e.g. *i.

In this case, you could use i->NotifyGovernanceVote(vote) instead of notifier. If the name is the only reason for this, you could just rename the iterator instead.

removed in 2957424 But now it won't compile 🙈

What is the error message?

At each point where notifier was replaced with the iterator, as you described some variant of below.

zmq/zmqnotificationinterface.cpp:174:17: error: request for member ‘Shutdown’ in ‘* it.std::_List_iterator<_Tp>::operator-><CZMQAbstractNotifier*>()’, which is of pointer type ‘CZMQAbstractNotifier*’ (maybe you meant to use ‘->’ ?) it->Shutdown(); ^

Ah, the list is a list of pointers. Try dereferencing the iterator first, then calling the method.

(*it)->Shutdown();

that did it 👍

nmarley · 2018-07-01T01:54:41Z

src/zmq/zmqnotificationinterface.cpp

+    for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i != notifiers.end(); )
+    {
+        CZMQAbstractNotifier *notifier = *i;
+        if (notifier->NotifyGovernanceVote(vote))


This syntax is ... ok I guess -- starting to get into personal preference a bit, but I really prefer to be able to use range-based for which is available as of C++11, and eliminate the if/else condition. Unfortunately that's not always the case, so I'd have to look into how std::list accepts an erasure to know if range-based for would work here.

nmarley · 2018-07-04T06:10:29Z

@paulied Yep, that works too. Thanks!

UdjinM6

utACK

UdjinM6

Heh, good catch! 👍
Tested, seems to be working as expected.

ACK

PastaPastaPasta · 2018-07-04T23:54:14Z

@UdjinM6 are votes coming through for you? I'm not getting any in my testing with jeffh

UdjinM6 · 2018-07-05T00:04:21Z

Yep, 2 terminals:

> ./src/dashd -testnet -zmqpubhashgovernancevote=tcp://127.0.0.1:28332

> ./contrib/zmq/zmq_sub.py 
- HASH GOVERNANCE VOTE (0) -
d4de8dcc005f5e7d3cfba3902c030d0e8fb1f8ddfe9c8c5494dd945a97000faf
- HASH GOVERNANCE VOTE (1) -
7041c977f9135513eb1f9ffcee838d98095221bff3d7dc6fa259bc0231dfda16
- HASH GOVERNANCE VOTE (2) -
bff11fa60da04a6d5b8d600dc91c73d856196ebeb96737691ce02881350956c9
...
- HASH GOVERNANCE VOTE (576) -
b0bb18a39333de20401fb15c08b89863b24e46c0b71dd381ef0b06bfdb0ad8e1
- HASH GOVERNANCE VOTE (577) -
1a631d0d794774108ebc626038b7128a39f0801eaee1f2e77c9d70c3f490a05e
...

jhodges10 · 2018-07-05T05:58:56Z

Votes are coming through correctly as notifications now for me, same with governance objects.

Currently trying to figure out why the serialized data doesn't match the serialization spec for raw votes.

PastaPastaPasta · 2018-07-05T19:12:10Z

Changed the location of the notification to one which I believe is slightly better. Both work but this one is more consistent based on where the govobject one is located. This will work just as well, since it is up the chain one call. it was prior in processvote, and was only called if that method will return true. And here it is called when processvote does return true

PastaPastaPasta · 2018-07-05T19:58:06Z

Implemented raw notifications, after jeff pushed me twords it.

Branch does compile, however the outputs from the raw messages do not match the formatting which serialization should follow as described in https://dash-docs.github.io/en/developer-reference#govobjvote. I'm not knowledgeable enough to know why...

UdjinM6 · 2018-07-05T22:20:50Z

Hmm... indeed. Should be no

00000000000000000000000000000000 ..... Collateral Hash

and

41 ................................... Signature length: 65

is missing in govobjvote. Probably a copy/paste error.

@thephez

UdjinM6

Tested, works as expected.

ACK

PastaPastaPasta · 2018-07-05T23:44:35Z

I don't follow @UdjinM6, are you saying that the serialization is correct, but the documentation is wrong? Based on the chart at the link above the serialization for a vote should be ~150 bytes but the output we got was 132 I think.

UdjinM6 · 2018-07-05T23:52:27Z

Yes, documentation has an error I described above. I tested 2 messages (1 govobj and 1 govobjvote) and they both had the serialization I expected (the one in SerializationOp() for the corresponding class). If you have troubles "deserializing" the output manually, you can post the string you have issues with here and I'll help breaking it in parts.

PastaPastaPasta · 2018-07-06T00:16:54Z

Here's a vote rawgovernancevote: dfe6fe221a75a1f88ddb56379b85947718472dab8767d7eaada457d1aa22eb6601000000f06ec371b287f6a04e967a65bf100d76412b23eaa21b42cc4f7a5838eb8fd31701000000030000004e4f3d5b00000000411c32f625a8b393159a929dbdc4960519010a6c33830c73ef77a370ab306a70e8217617d1266cb5935f9663b69529f56cc358899ef8754d6d4351001102c61a4dbf

How would you go about breaking that down

thephez · 2018-07-06T00:18:34Z

@paulied @jhodges10 @UdjinM6 Yeah, my bad. Sorry about that. I think the table showing the fields is correct, but the hexdump is wrong.
Here is what one really looks like...

thephez · 2018-07-06T00:34:58Z

@paulied

dfe6fe221a75a1f88ddb56379b85947718472dab8767d7eaada457d1aa22eb6601000000f06ec371b287f6a04e967a65bf100d76412b23eaa21b42cc4f7a5838eb8fd31701000000030000004e4f3d5b00000000411c32f625a8b393159a929dbdc4960519010a6c33830c73ef77a370ab306a70e8217617d1266cb5935f9663b69529f56cc358899ef8754d6d4351001102c61a4dbf broken down:

Outpoint (Size 36) -

    dfe6fe221a75a1f88ddb56379b85947718472dab8767d7eaada457d1aa22eb66 (Hash - 32 bytes)
    01000000 (Index 1 - 4 bytes)

f06ec371b287f6a04e967a65bf100d76412b23eaa21b42cc4f7a5838eb8fd317 (Parent hash - 32 bytes)
01000000 (Outcome - 0x00000001 = 1 = yes - 4 bytes)
03000000 (Signal - 0x00000003 = 3 = delete - 4 bytes)
4e4f3d5b00000000 (Time - 0x000000005b3d4f4e = 1530744654 - unix epoch 1530744654 = Wed Jul 04 22:50:54 2018 UTC - 8 bytes)
41 - Signature length (0x41 = 65 - 1 byte)
1c32f625a8b393159a929dbdc4960519010a6c33830c73ef77a370ab306a70e8217617d1266cb5935f9663b69529f56cc358899ef8754d6d4351001102c61a4dbf (Signature - 65 bytes)

Note the endian-ness. Gets confusing if you don't flip the byte order 🙂

PastaPastaPasta · 2018-07-06T00:48:48Z

Thanks @thephez!

@nmarley

* fix whitespace * added zmq stuff for governance objects and votes it seems that zmq is currently not in a working state, need to figure that out. Need to: plug in the new methods added possibly plug in the old methods, as it doesn't look like they are. * formatting fix. Will probably need to revert for this PR * continue linking new zmq messages in * added comment, might need to revert * fixes error of it not knowing about the classes * Actually link in, all new govobjects and govvotes should be broadcast over zmq now. * fix compile error, forgot to change params when copying * fix compile error * add imports to the header files in zmqconfig.h * fixing linking(i think) error * Revert "added comment, might need to revert" This reverts commit 2918ea4. * Revert "formatting fix. Will probably need to revert for this PR" This reverts commit ca10558. * fix tabs etc * EOL added * optimization of hash.begin() @nmarley thoughts? * remove formatting changes * iterator i -> it and removal of notifier * typo in df879f5 * use auto for the iterators * introduce hash prefix * implement changes in zmq_sub.py, update the doc, and change argument name to fix typo * deref iterators before calling methods * continued e8a4c50 * missed one... continued e8a4c50 * killing some tabs * fix spacing for setting or comparing iterators * change order of new variables to match current setup * re-add elif's I didn't realize got removed * Revert "fix spacing for setting or comparing iterators" This reverts commit 8ce2068. * Revert "use auto for the iterators" This reverts commit cb16cf0. * Revert "missed one... continued e8a4c50" This reverts commit 2087cf8. * Revert "continued e8a4c50" This reverts commit a78c8ad. * Revert "deref iterators before calling methods" This reverts commit e8a4c50. * Revert "iterator i -> it and removal of notifier" This reverts commit 2957424. * Revert "fix whitespace" This reverts commit 612be48. * Revert "typo in df879f5" * Revert "Optimization of hash.begin()" * fixes problem with revert * Udjin complain's a lot ;) * help text, init.cpp * add signals in validationinterface.cpp * Change location of vote notification call. * remain consistent * remove unneeded include due to change of notification location * implement raw notifications

PastaPastaPasta and others added 13 commits June 30, 2018 01:34

fix whitespace

612be48

added zmq stuff for governance objects and votes

6a0df48

it seems that zmq is currently not in a working state, need to figure that out. Need to: plug in the new methods added possibly plug in the old methods, as it doesn't look like they are.

formatting fix. Will probably need to revert for this PR

ca10558

continue linking new zmq messages in

96d3b7e

added comment, might need to revert

2918ea4

fixes error of it not knowing about the classes

a7437bc

Actually link in, all new govobjects and govvotes should be broadcast…

7a4220c

… over zmq now.

fix compile error, forgot to change params when copying

ebe77f5

fix compile error

fc5cd47

add imports to the header files in zmqconfig.h

f63fd97

fixing linking(i think) error

44291d7

Revert "added comment, might need to revert"

8143bf3

This reverts commit 2918ea4.

Revert "formatting fix. Will probably need to revert for this PR"

48b579c

This reverts commit ca10558.

PastaPastaPasta commented Jul 1, 2018

View reviewed changes

src/zmq/zmqpublishnotifier.h Show resolved Hide resolved

fix tabs etc

97ee91b

PastaPastaPasta force-pushed the zmq branch from af3c000 to 97ee91b Compare July 1, 2018 00:35

PastaPastaPasta commented Jul 1, 2018

View reviewed changes

nmarley reviewed Jul 1, 2018

View reviewed changes

UdjinM6 added this to the 12.4 milestone Jul 4, 2018

UdjinM6 previously approved these changes Jul 4, 2018

View reviewed changes

UdjinM6 changed the title ~~[WIP] Implement Governance into the ZMQ messages~~ Implement Governance into the ZMQ messages Jul 4, 2018

add signals in validationinterface.cpp

5d738a9

PastaPastaPasta dismissed UdjinM6’s stale review via 5d738a9 July 4, 2018 22:49

UdjinM6 previously approved these changes Jul 4, 2018

View reviewed changes

Change location of vote notification call.

a3aea25

PastaPastaPasta dismissed UdjinM6’s stale review via a3aea25 July 5, 2018 19:11

PastaPastaPasta added 3 commits July 5, 2018 14:17

remain consistent

3cc64e5

remove unneeded include due to change of notification location

f7c8863

implement raw notifications

e9af78f

PastaPastaPasta force-pushed the zmq branch from cf73ffe to e9af78f Compare July 5, 2018 19:51

UdjinM6 approved these changes Jul 5, 2018

View reviewed changes

thephez mentioned this pull request Jul 6, 2018

Content - Fix govobjvote hexdump dash-docs/dash-docs#81

Merged

UdjinM6 merged commit 0e68934 into dashpay:develop Jul 12, 2018

PastaPastaPasta deleted the zmq branch July 13, 2018 02:20

Implement Governance into the ZMQ messages #2160

Implement Governance into the ZMQ messages #2160

Uh oh!

Conversation

PastaPastaPasta commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Jul 1, 2018 • 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!

Uh oh!

MrDefacto commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrDefacto commented Jul 1, 2018

Uh oh!

PastaPastaPasta commented Jul 1, 2018

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!

MrDefacto commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Jul 1, 2018 • 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!

nmarley Jul 1, 2018 • 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!

PastaPastaPasta Jul 1, 2018 • 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!

nmarley Jul 1, 2018 • 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!

PastaPastaPasta Jul 1, 2018 • 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!

PastaPastaPasta Jul 2, 2018 • 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!

nmarley commented Jul 4, 2018

Uh oh!

UdjinM6 left a comment

PastaPastaPasta Jul 1, 2018 •

edited

Loading

PastaPastaPasta Jul 1, 2018 •

edited

Loading

nmarley Jul 1, 2018 •

edited

Loading

PastaPastaPasta Jul 1, 2018 •

edited

Loading

nmarley Jul 1, 2018 •

edited

Loading

PastaPastaPasta Jul 1, 2018 •

edited

Loading

PastaPastaPasta Jul 2, 2018 •

edited

Loading

jhodges10 commented Jul 5, 2018 •

edited

Loading

PastaPastaPasta commented Jul 5, 2018 •

edited

Loading

UdjinM6 commented Jul 5, 2018 •

edited

Loading

PastaPastaPasta commented Jul 6, 2018 •

edited

Loading

thephez commented Jul 6, 2018 •

edited

Loading