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

Add support for raw script addresses #587

Merged
merged 4 commits into from
Oct 12, 2019

Conversation

aguycalled
Copy link
Member

This PR adds support for a new type of addresses which encode raw scripts.

A new rpc command has been added createrawscriptaddress.

Example: createrawscriptaddress 6ac1 outputs 3HnzbJ4TR9. Sending coins to 3HnzbJ4TR9 would send them to the DAO Fund.

This enables to easily redirect stake rewards to any special script (like the DAO fund).

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

testedACK (Ubuntu 19.10)

Test makes sense and passes.

@R-Marino
Copy link
Contributor

I think the new functionality is useful, especially for experimenting with smart contracts more easily.

However, it seems to me that in its current form is a bit risky, the user does not know if the address obtained expresses a correct script.
Using an invalid script in a transaction you risk "burning" the coins sent, without the possibility of recovery.

Currently the user could get an address that is actually based on an input:
    1) Invalid or not correctly interpreted as hexadecimal value.
        Example: createrawscriptaddress 0x6ac1
                 7rLnRLD
    2) valid, but the script it represents is incorrect and cannot be decoded.
        Ex: createrawscriptaddress "216ac1"
            B4LorUWVZ8x
            decodescript "216ac1"
            {
            "asm": "[error]",
            "type": "nonstandard",
            "p2sh": "2MziQuEaXPo8MvB7QXW5Ck3icaThAa4cYSD"
            }
            
    3) formally decodable, but which uses OP_CODE still undefined or removed
        Ex: createrawscriptaddress "f06ac1"
            BCBnKiqpbsY
            decodescript "f06ac1"
            {
            "asm": "OP_UNKNOWN OP_RETURN OP_CFUND",
            "type": "nonstandard",
            "p2sh": "2NChNGPMEnBaTcjAoc2JhBfLfgnVhD7JTDD"
            }

Case three could be useful in testing (devnet, testnet), but it should never be allowed in the main chain.

IMHO, If the RPC was expanded so as to return addresses only when the scripts are totally valid, for example by integrating the decodescrip function, it will be an excellent improvement.

@mxaddict
Copy link
Contributor

@aguycalled I think @R-Marino has a good point about integrating the decode script into this 👍

@aguycalled
Copy link
Member Author

thanks for the feedback. even if additional checks are implemented, an user will still be able to create addresses for unspendable scripts (OP_RETURN the most obvious example)
@R-Marino i find your suggestions greatly valuable. I've just added a few sanity checks in line with your comments

@R-Marino
Copy link
Contributor

Very well, great job.

You are absolutely right. We as users, will always be able to find new, more sophisticated and "smart" ways of burning our money. ;-P
However, one thing is to do this fully aware of what one does, another thing is to make mistakes just for a little carelessness.
So thanks to you for promptly taking care of this.


Shortly after writing, I read a bitcoin news, which in some way could be linked to this PR.
The news talk about Miniscript, a kind of formal language for creating bitcoin smart contract scripts.
It is here:
https://www.coindesk.com/pieter-wuille-unveils-miniscript-a-new-smart-contract-language-for-bitcoin
http://bitcoin.sipa.be/miniscript/

Surely this extension goes far beyond the scope of this PR, and I imagine that its possible inclusion in NAV core wallet would also require some work for integration and adaptation to some specific NAV_OP_Codes.
I also think that being a very technical topic, surely the users really interested in using it would only be a few dozen.
So I don't think it's a priority, but in some way it could be very interesting for the future.
IMHO, The proper integration of that commit into another rpc, or even into an external module could be the perfect complement to this PR.

@aguycalled
Copy link
Member Author

if i understand it correctly, miniscript is just a higher level language for the traditional bitcoin script language. think miniscript is javascript and script is assembly. miniscript translates a set of combined logic conditions into the shortest/cheaper script code. the first is easier to construct for a common human.

as this happens in higher layers, there's nothing needed to implement at the node layer more than maybe adding an interpreter for the sake of facilitating user interaction as long as the set of needed op codes is shared between bitcoin and navcoin.

@R-Marino
Copy link
Contributor

Well, more than assembler, the script reminds me more of Forth, or RPN,
but you're right, it is a interpreter after all.

But the OP_CODE code set is not totally shared between Bitcoin and NavCoin,
for example we have OP_COINSTAKE, OP_CFUND. etc. until OP_POOL.

Even now we could think of singular things, for example by combining
OP_COINSTAKE and OP_CHECKLOCKTIMEVERIFY we could obtain something analogous
to a kind of coldstake with "long-term storage", that is capable of producing
stakes, but that cannot be spent before a certain date.

Or by combining differently, a "Slow ColdStake", which can always be spent
but unable to produce stakes before a given time has passed.

This would already be something, but if you then create OPCODE able to give
information on the present state, or the status of the other transactions,
then you would see really smart contracts.

Try to imagine for example a opcode that is able to verify if the coinbase
votes for a proposal, after all it is an internal state regarding the same
block.

Already this would open up other opportunities, for example you could have
a "ColdStake for a cause", so as to be certain that a pool could produce one
stakes, only when it reflects enterely the type of vote of the owner.

If instead the opcode could control the outputs of the stake, you could get
a "ColdStake for charity", I mean that the stake would be possible only when
a part of the reward went with absolute certainty to a given address.

These hypotheses probably do not all have a practical sense, or they may have
more opposed than positive aspects, but they are only examples of types of
smart contract based on OP_COINSTAKE that could be quite unique,
unique to Nav.

Sorry, maybe I have digress a little too much.

@mxaddict
Copy link
Contributor

mxaddict commented Sep 4, 2019

@aguycalled @R-Marino are there more changes to be added to this branch? Or is it ok to review now?

@mxaddict
Copy link
Contributor

@aguycalled build and tested on Ubuntu 19.10, works fine, I also tested this merged with master and set the contribution address to Community Fund and it was sending the stakes correctly to CFUND

@mxaddict
Copy link
Contributor

Tested sending, receiving, staking and generating block in QT and all were working fine.

@mxaddict
Copy link
Contributor

Added needed for final release tag as the staking split feature is kinda missing this PR to work 100%.

@proletesseract
Copy link
Member

@mxaddict the sanity checks on the RPC command added here 0088e93 should be enough for now. It at least checks the parsed script is valid hex and exists in the OP_CODE list.

@proletesseract
Copy link
Member

Tested on OSX,

  • compiles & runs
  • unit test passes
  • RPC rejects non hex values
  • RPC rejects hex values which are not mapped to an OP_CODE
  • wallet recognises addresses as valid to send to.

@proletesseract proletesseract merged commit 49f7408 into navcoin:master Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants