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

Superblock Contract Forwarding #1060

Merged
merged 20 commits into from
Aug 13, 2018

Conversation

tomasbrod
Copy link
Member

@tomasbrod tomasbrod commented Apr 22, 2018

This all works only when superblock is needed and when not disabled by -supercfwd=0.

When node discovers a peer that has with neural hash that is currently popular, it requests neural contract from that peer. When contract is received it is checked and saved in memory. When this node stakes, and it's local NN contract is empty, it uses the forwarded contract to stake superblock.

Netcmd neural/neural_hash is used to discover peers with contract. This netcmd is supported by current nodes (without this pr). In this PR, special flag and popular neural hash is appended. This allows nodes with this PR to send us contract with requested hash, if they have it, compressed in the same response. The contract is sent in neural/supercfwdr netcmd to avoid possible NN confusion possibly caused by forwarded contract. Otherwise (without this PR), peers just send their local neural hash, and if it matches consensus, node requests the contract using neural/quorum netcmd. To this command quorum_nresp is generally generated, with the contract uncompressed.

When node receives a valid contract with hash in consensus, apart from saving the contract (compressed), it also notifies its peers, so they can possibly request the contract, if they want. The notification is done with neural/neural_hash netcmd to nodes that are Participating but do not have a NN. The notification is not done when -listen=0, beacause users with listen disabled usually want to save bandwidth.

The Super-majority vote counting mechanism is left still broken, because changing it would be hard-fork.

And as always, I spent too much on this even moreso this is only temporary solution and very far from ideal.

@tomasbrod
Copy link
Member Author

note: I found that GetCurrentNeuralNetworkSupermajorityHash is actually used in FullSyncWithDPORNodes, so the removal of it is errorneous.

@tomasbrod
Copy link
Member Author

bug found, hold on merge

@tomasbrod
Copy link
Member Author

corrected the compare mistake, added locks and
added notify other nodes to good contract

@denravonska
Copy link
Member

Here is my start at the same functionality. What I like about it is that it either gets the contract from the NN or via a relay but other than that the code stays the same. It is not as feature complete as @tomasbrod's implementation though.

@tomasbrod
Copy link
Member Author

Your approach to hook into existing code is more straightforward and also shorter. I edited what I was more familiar with. It too does either get the contract from NN call or from relay cache.

void QuorumResponseHook(CNode* fromNode, const std::string& neural_response);
void SendResponse(CNode* fromNode, const std::string& req_hash);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no apparent reason to keep this in miner. Can it be moved to another file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it can be moved, and probably should. I put it in miner, because it was related to adding neural contracts to mined blocks. If you are talking about the declarations being in the .h file, they are there because the functions are defined in the .cpp file.

@@ -7116,6 +7116,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
}
else if (neural_request=="neural_hash")
{
if(0==neural_request_id.compare(0,13,"supercfwd.rqa"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this be compared directly? In miner.cpp:686 it looks like "supercfwd.rqa" is the entire content of the request id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for extensibility. I am re-using the request-ID field and there might be a need for a actual request ID.

std::string sBinContract;
bool fEnable(false);

int RequestAnyNode(const std::string& consensus_hash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be in net.cpp or maybe superblock.cpp?

@tomasbrod
Copy link
Member Author

tomasbrod commented Jul 29, 2018

Hey, if you want to move the functions, you can ask me or move them yourself. I ask, because I do not know what to move where. Create a new file?

if(req_hash==sCacheHash)
{
if(fDebug10) LogPrintf("supercfwd.SendResponse: %s requested %s, sending forwarded binary contract (size %d)",fromNode->addrName,req_hash,sBinContract.length());
fromNode->PushMessage("neural", std::string("supercfwdr"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of this scope, but what do you say we make these message constants later?

const std::string SUPERBLOCK_FWD_RESP("supercfwdr");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely!
The "message type" (first string in message, saying what it is) should even be 8-bit integer constant.

@denravonska denravonska merged commit 81e59cf into gridcoin-community:development Aug 13, 2018
Move away from xml contracting and tx data field automation moved this from To do to Done Aug 13, 2018
denravonska added a commit to denravonska/Gridcoin-Research that referenced this pull request Aug 14, 2018
denravonska added a commit that referenced this pull request Oct 19, 2018
Added:
 - Linux nodes can now stake superblocks using forwarded contracts #1060 (@tomasbrod).

Changed:
 - Replace interest with constant block reward #1160 (@tomasbrod).
   Fork is set to trigger at block 1420000.
 - Raise coinstake output count limit to 8 #1261 (@tomasbrod).
 - Port of Bitcoin hash implementation #1208 (@jamescowens).
 - Minor canges for the build documentation #1091 (@Lenni).
 - Allow sendmany to be used without an account specified #1158 (@Foggyx420).

Fixed:
 - Fix `cpids` and `validcpids` not returning the correct data #1233
   (@Foggyx420).
 - Fix `listsinceblock` not showing mined blocks to change addresses #501 (@Foggyx420).
 - Fix crash when raining using a locked wallet #1236 (@Foggyx420).
 - Fix invalid stake reward/fee calculation (@jamescowens).
 - Fix divide by zero bug in `getblockstats` RPC #1292 (@Foggyx420).
 - Bypass historical bad blocks on testnet #1252 (@Quezacoatl1).
 - Fix MacOS memorybarrier warnings #1193 (@ghost).

Removed:
 - Remove neuralhash from the getpeerinfo and node stats #1123 (@Foggyx420).
 - Remove obsolete NN code #1121 (@Foggyx420).
 - Remove (lower) Mint Limiter #1212 (@tomasbrod).
denravonska added a commit to denravonska/Gridcoin-Research that referenced this pull request Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants