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 staking #7

Merged
merged 12 commits into from
Nov 26, 2019
Merged

add staking #7

merged 12 commits into from
Nov 26, 2019

Conversation

buck54321
Copy link
Member

Adds widespread but not-quite-complete support for all script types other that alt-sig types. Implementation is a mirror of parts of dcrd's txscript and dcrutil modules.

Adds a stake pool client (StakePool class) with methods for getting the purchase info, getting the stats, setting the voting address, and setting the vote bits.

New http module in util used to be the networking portion of dcrdata module, but is now shared among some others.

Adds ticket purchase support (VSP only). Also moves pydecred tests to a dedicated test file to clean up packages for import. This should be done in the crypto package too, but I'm leaving that for another PR.

Updated GUI interface to support stake pool handling and ticket purchasing.

image

The main goal of this effort was to reproduce dcrwallet's (*Wallet).purchaseTickets method. The mirrored method is implemented on the DcrdataBlockchain, with keys and internal addresses coming from a closure passed from an unlocked wallet.

Notably, no way to revoke a missed or expired ticket yet. Will follow up for that in another PR. This one is already way too big.

PR is still a draft for now while I continue testing and tweaking, but it's definitely ready for scrutiny. I'll add examples before removing from draft status, too.

}
@staticmethod
def __fromjson__(obj):
acct = Account(
def __fromjson__(obj, cls=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

Passing the constructor like this is a little bit of a hack. Once I have more than one account, I'll put some more thought into this.

# depth = obj["depth"],
# childNum = obj["childNum"],
# isPrivate = obj["isPrivate"],
# )
Copy link
Member Author

@buck54321 buck54321 Nov 13, 2019

Choose a reason for hiding this comment

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

ExtendedKey should never be saved or JSON-encoded. The key is serialized and encrypted before saving.


The DecredAccount inherits from the tinydecred base Account and adds staking
support.
"""
Copy link
Member Author

Choose a reason for hiding this comment

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

In preparation for multi-asset, I've separated account into a base class and an inheriting Decred-specific class.

# In addition to the standard internal and external branches, we'll have a third
# branch. This should help accomodate upcoming changes to dcrstakepool. See also
# https://github.com/decred/dcrstakepool/pull/514
STAKE_BRANCH = 2
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if there is a best way to handle this. I've gone off-BIP a little bit to create a staking branch from which all ticket-related keys will be generated. Right now, only the zeroth key is used, but more will be used when accountless VSP is implemented.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this was the method discussed. AFAIK still waiting on @jrick's opinion.

Comment on lines 347 to 380
addrs = self.unseenAddrs()
while addrs:
for addr in addrs:
for txid in blockchain.txsForAddr(addr):
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 can potentially take a long time since the requests must be made one address at-a-time. Might consider breaking the sync into two sections, one quick but critical section and one deep sync section that can run in a background thread afterwards.

scriptPubKey = changeScript,
amount = changeAmount*1e-8,
satoshis = changeAmount,
))

return newTx, utxos, newUTXOs

class TestDcrdata(unittest.TestCase):
Copy link
Member Author

Choose a reason for hiding this comment

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

All testing classes should be moved to separate modules.

"""
w, lyt = makeWidget(QtWidgets.QWidget, HORIZONTAL)
lyt.addWidget(wgt)
w.setContentsMargins(l, t, r, b)
return w


class QSimpleTable(QtWidgets.QTableWidget):
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused and cumbersome.

@@ -137,7 +145,7 @@ def showEvent(self, e):
def closeEvent(self, e):
self.hide()
e.ignore()
def stack(self, w):
def stack_(self, w):
Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to connect any common UI-modifying functions as Qt slots to simplify threaded operations.

ui/screens.py Outdated
is based purely on voting record, e.g. voted/(voted+missed). The sorting
and some initial filtering was already performed in setPools.
"""
count = len(self.pools)//2+1
Copy link
Member Author

@buck54321 buck54321 Nov 13, 2019

Choose a reason for hiding this comment

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

This is a simple but pretty dumb way to filter low-performing pools. Consider other options.

util/tinyjson.py Outdated
@@ -11,7 +11,7 @@
def clsKey(cls):
return cls.__qualname__

def register(cls):
def register(cls, tag=None):
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 enables setting the __jsontag__ manually, instead of using the built in name generating algorithm. Needed if registering identically-named classes.

@buck54321 buck54321 marked this pull request as ready for review November 13, 2019 23:32
@JoeGruffins
Copy link
Member

It keeps growing.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

BIG.

So, this is a fist pass. It looks like accounts moved after I had left some comments, at least one was already addressed, but ignore any that are no longer pertinent...

When trying to run app.py now, it says my password is incorrect.

log
2019-11-15 15:30:10,441 app INFO <module>(523) configuration file at /home/joe/.local/share/TinyDecred/tinywallet.conf
2019-11-15 15:30:10,441 app INFO <module>(524) data directory at /home/joe/.local/share/TinyDecred
  File "app.py", line 185, in openWallet
    w = Wallet.openFile(path, pw)
  File "/home/joe/.local/lib/python3.8/site-packages/tinydecred/wallet/wallet.py", line 178, in openFile
    wallet = tinyjson.load(fileKey.decrypt(bytes.fromhex(wrapper["wallet"])).decode())
  File "/home/joe/.local/lib/python3.8/site-packages/tinydecred/util/tinyjson.py", line 37, in load
    return json.loads(s, object_hook=decoder)
  File "/usr/lib/python3.8/json/__init__.py", line 370, in loads
    return cls(**kw).decode(s)
  File "/usr/lib/python3.8/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python3.8/json/decoder.py", line 353, in raw_decode
    obj, end = self.scan_once(s, idx)
  File "/home/joe/.local/lib/python3.8/site-packages/tinydecred/util/tinyjson.py", line 30, in decoder
    return _types[obj["_jt_"]].__fromjson__(obj)
2019-11-15 15:30:14,836 app WARNING openWallet(191) exception encountered while attempting to open wallet: 'Account'
None

Going back to master I can proceed as normal.
I can also press the back arrow at any time and see this:

wallet

wallet

but then I cannot return to the password input screen.

I will try moving my wallet and making a new one.

Removing the wallet.db from the testnet folder has no affect.
After removing the wallet, I can still open it on master. Where is it?

That back arrow is also present in master

Found the real wallet file! Deleted and made a new one. Seems to be working alright now.
Will continue testing.

Should I be able to get to some staking screen? :P

I get to this screen, which way to staking?

wallet

newbutton png

accounts.py Outdated
Comment on lines 393 to 400
def getUTXOs(self, requested, approve=None):
"""
Find confirmed and mature UTXOs, smallest first, that sum to the
requested amount, in atoms.

Args:
requested (int): Required amount in atoms.
filter (func(UTXO) -> bool): Optional UTXO filtering function.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the second argument's name is "approve" and not "filter".

self.lastInternalIndex = idx
return addr
newAddrs = []
extAddrs, intAddrs = self.externalAddresses, self.internalAddresses
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of the purpose of this. It's not a copy correct? Is it to obtain shorter names? If so you use self.internalAddresses once below.

pydecred/account.py Outdated Show resolved Hide resolved
pydecred/account.py Show resolved Hide resolved
pydecred/account.py Outdated Show resolved Hide resolved
pydecred/txscript.py Show resolved Hide resolved
pydecred/txscript.py Show resolved Hide resolved
pydecred/txscript.py Show resolved Hide resolved
pydecred/txscript.py Outdated Show resolved Hide resolved
pydecred/txscript.py Show resolved Hide resolved
@xaur
Copy link

xaur commented Nov 15, 2019

Adds a stake pool client (StakePool class)

The rename to "VSP" was almost universal so how about using these terms in code as well?

def test_extract_script_addrs(self):
from tinydecred.pydecred import mainnet
scriptVersion = 0

Copy link
Member

@JoeGruffins JoeGruffins Nov 16, 2019

Choose a reason for hiding this comment

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

This is really my first time looking at a python test, but it looks like you need to add:

if __name__ == '__main__':
    unittest.main()

To the bottom in order to test as per:
https://docs.python.org/3/library/unittest.html

After adding those lines, I got this:

test results
$ python3 pydecred/tests.py 
.Emsg: {'event': 'subscribeResp', 'message': {'success': True, 'request_event': 'address:Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqkx', 'request_id': 6, 'data': 'subscribed to address:Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqkx'}}
msg: {'done': 'done'}
. no stake pool credentials provided. skipping stake pool test
s no stake pool credentials provided. skipping stake pool test
s no stake pool credentials provided. skipping stake pool test
s...............
======================================================================
ERROR: test_purchase_ticket (__main__.TestDcrdata)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pydecred/tests.py", line 1974, in test_purchase_ticket
    ticketPrice = self.stakeDiff()
AttributeError: 'TestDcrdata' object has no attribute 'stakeDiff'

----------------------------------------------------------------------
Ran 21 tests in 10.503s

FAILED (errors=1, skipped=3)

edit: It seems test can also be run from the command line like so, without adding to the file:

python3 -m unittest discover

The result is the same

Copy link
Member Author

Choose a reason for hiding this comment

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

I typically test by specifying the module, e.g.

python -m unittest crypto

Looks like you're looking into streamlining testing in #18, so I'll let you find the best solution for our needs.

Copy link
Member Author

Choose a reason for hiding this comment

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

stakeDiff error fixed.

@buck54321
Copy link
Member Author

I'll do class StakePool -> class VotingServiceProvider, but I'm gonna use stakePool many places internally because vsp is non-descriptive and votingServiceProvider is too long.

@buck54321
Copy link
Member Author

buck54321 commented Nov 18, 2019

re wallet won't load: At this point of development, expect every major PR to break the wallet, so delete the wallet and reload with your mnemonic seed. In the future, we'll need to version the wallet and handle upgrades with more care.

@JoeGruffins
Copy link
Member

JoeGruffins commented Nov 20, 2019

Working great!
Just a few things.
One is that the "show another" button doesn't seem to do anything
Another is that % staked did not change until I restarted the wallet once, when starting from balance 0.
And just a feeling, but startup seems to take considerably longer after buying 34 tickets.
And right after purchasing a ticket, my the spent dcr is added to the totat amout before being marked as spent. This wallet only has 500.99 dcr in it, just after buying some tickets the amount shoots up for a short time (first I bought 11 tickets, this is right after buying 23 more):

wallet

toomanydcr

You can also see that percent staked is 0 although 98% of funds have been staked. The correct percent shows after a restart.

One more very tiny thing, the vsp url doesn't seem to work if there is a trailing slash at the end, perhaps that can be removed automatically?

I just had some tickets that looked like they were bought, but apparently were not, the log:

log
2019-11-20 14:40:01,481 dcrdata ERROR broadcast(929) broadcast error: ('RequestError', 'Error encountered in requesting path https://testnet.dcrdata.org/insight/api/tx/send: HTTP Error 400: Bad Request\nNone')
2019-11-20 14:40:02,160 dcrdata ERROR broadcast(929) broadcast error: ('RequestError', 'Error encountered in requesting path https://testnet.dcrdata.org/insight/api/tx/send: HTTP Error 400: Bad Request\nNone')
2019-11-20 14:40:02,169 dcrdata INFO purchaseTickets(1346) published ticket 4d4de58ba2ac8ea794a24e3495a4aa2f1ecb523565fcb54efeeea48cbb075210
2019-11-20 14:40:02,893 dcrdata ERROR broadcast(929) broadcast error: ('RequestError', 'Error encountered in requesting path https://testnet.dcrdata.org/insight/api/tx/send: HTTP Error 400: Bad Request\nNone')
2019-11-20 14:40:02,902 dcrdata INFO purchaseTickets(1346) published ticket 30a0c3aca91dbb10e0095e5da9413216870497feea4595d10a9d37a60a29aaee
2019-11-20 14:40:03,630 dcrdata ERROR broadcast(929) broadcast error: ('RequestError', 'Error encountered in requesting path https://testnet.dcrdata.org/insight/api/tx/send: HTTP Error 400: Bad Request\nNone')
2019-11-20 14:40:03,638 dcrdata INFO purchaseTickets(1346) published ticket 1c57e8bd8e15d276fae28400b2f8f5dc78a080161770341a208d3e9f27b67f4b

I can't remember, but does this check to see if we are in the ticket buying window? Also, is the ticket price and such recalculated if there is a change from the time of entering the vsp screen?

Some more balance discrepancies, these don't need to be fixed now, but should make an issue to make syncing better.
Stake some ~36 tickets, close laptop for about a day, open to this amount:

open laptop

before

Restart wallet.

after restart

before2

After sync completes. Presumably this is the correct amount.

after sync

after

Adds support for all script types other that alt-sig types. Implementation
is a mirror of parts of txscript and dcrutil modules. Also adds basic
stake pool client.

Adds new http module in util, used in both dcrdata and stakepool.
Also moves pydecred tests to dedicated test file so they aren't
evaluated on import.
Updated interface to support ticket purchases via VSP. No solo voting
and still needs revocation support, but signing stake-P2PKH outputs
of all types is now supported.
Comment on lines 363 to 368
# Remove spent utxos from cache.
self.spendUTXOs(spentUTXOs)
# Add new UTXOs to set. These may be replaced with network-sourced
# UTXOs once the wallet receives an update from the BlockChain.
for utxo in newUTXOs:
self.addUTXO(utxo)
Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting for the updates from dcrdata here instead. This should help with some of the transient balance display issues.

The balance display being wrong for a moment at startup is because the balance is being stored as part of the encrypted wallet object, so when an update to the balance occurs while the wallet is locked, the new value is not properly stored. I have a plan, but it should wait until the database changes proposed in the budget proposal are complete.

@@ -927,7 +927,7 @@ def broadcast(self, txHex):
return True
except Exception as e:
log.error("broadcast error: %s" % e)
return False
raise e
Copy link
Member Author

Choose a reason for hiding this comment

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

Propagate broadcast exceptions.

Comment on lines +1330 to +1331
if cfg.net.Name == "mainnet":
self.pools = [p for p in pools if tNow - p["LastUpdated"] < 86400 and self.scorePool(p) > 95]
Copy link
Member Author

Choose a reason for hiding this comment

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

Pools have a simple filter for inactivity or really poor performance. Unfortunately, 2 of the 3 listed testnet VSPs do not currently pass. Disabling filtering for testnet for now.

@buck54321
Copy link
Member Author

@JoeGruffins

I was able to improve some of the undesirable behavior you were seeing. Some others will get better once the database work is done and the storage is refactored.

I am unable to reproduce your broadcast issues, but the errors will propagate to the UI now. Let's chat on matrix and see if we can figure out how to reproduce.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

The percent staked still doesn't change until after a restart.

But other than that it's working great! LGTM

Allow crappy pools, lol. Could add my testnet pool, it's not crappy...

@@ -461,8 +461,7 @@ def __init__(self, app):
optsLyt.addWidget(self.spinner, 0, 1, Q.ALIGN_RIGHT)

# Open staking window. Button is initally hidden until sync is complete.
self.stakeBttn = btn = app.getButton(SMALL, "Staking")
btn.setVisible(False)
btn = app.getButton(SMALL, "Staking")
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to hide the staking button anymore with the changes in this PR. This and the gap address synchronization handling should make the sync much faster.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Doubly approved.

@buck54321 buck54321 merged commit a7d871c into decred:master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants