dashpay / dash

Dash - Reinventing Cryptocurrency
https://www.dash.org
MIT License
1.49k stars 1.2k forks source link

Modify ZMQ notification `rawchainlocksig` to only include CLSIG, and not include the raw block #3713

Open PastaPastaPasta opened 4 years ago

PastaPastaPasta commented 4 years ago

Currently rawchainlocksig includes the serialized raw block and the serialized CLSIG. Change it to only include the serialized CLSIG

If an integration needs the raw block, they should use rawblock zmq notification in conjunction with rawchainlocksig. Or if they want raw blocks, but only once they've been chainlocked, they can use rawchainlock zmq notification

thephez commented 4 years ago

Before we remove the raw block, do we have any idea:

  1. Why it was put there originally?
  2. How likely it is we cause issues for an existing ChainLock integration by doing this?
PastaPastaPasta commented 4 years ago

It was implemented by @UdjinM6, so it'd be good to ask him. Overall I think the risk is low that someone is using that zmq notification, I guess we could keep that zmq as is, maybe depreciate it in v17 and add rawchainlocksigonly that doesn't include the block?

xdustinface commented 4 years ago

Well actually i think it makes sense to have one command which provides block + CLSIG. I just feel like the naming isn't the best there..

Currently we have:

Maybe we should just add one command and rename the others.. like

How about that?

Edit: About the integration partners.. im not sure if those listeners were published/promoted anywhere else already or if they appear in our docs. They don't appear in the command line description yet aka hidden features :D So at least for me it feels like chance of we break someone's integrations with it are very low at this point.

Edit Edit: Ok.. they are written down in the ZMQ documentation so maaaybe chances are bit higher as i thought :P

UdjinM6 commented 4 years ago

My (old) thoughts re zmq message names: https://github.com/dashpay/dash/pull/2930#issuecomment-494916065. TL;DR: Keep old messages as is for now (v0.17 atm) and warn about their future deprecation (say in v0.18 or v0.19, should be enough time to update imo). Introduce new messages in parallel, ideally with short yet descriptive and correct names.

I would probably also think about switching to snake case to improve readability while at it (unless I'm violating some zmq naming convention, wasn't able to find anything about it though) e.g.:

hashchainlock   -> hash_locked_block
hashtxlock      -> hash_locked_tx
rawchainlock    -> raw_locked_block
rawchainlocksig -> raw_locked_block_and_sig
rawtxlock       -> raw_locked_tx
rawtxlocksig    -> raw_locked_tx_and_sig

(+ apply snake case to all other Dash specific messages ~, including legacy bitcoin ones to ensure consistency i.e. hash_block, hash_tx etc.~).

EDIT: fixed

PastaPastaPasta commented 4 years ago

Hmm, @UdjinM6 I don't love touching all the existing zmq commands, as that would potentially break support with applications that would otherwise support dash out of the box.