-
Notifications
You must be signed in to change notification settings - Fork 14
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
add staking #7
Conversation
} | ||
@staticmethod | ||
def __fromjson__(obj): | ||
acct = Account( | ||
def __fromjson__(obj, cls=None): |
There was a problem hiding this comment.
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"], | ||
# ) |
There was a problem hiding this comment.
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. | ||
""" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pydecred/account.py
Outdated
addrs = self.unseenAddrs() | ||
while addrs: | ||
for addr in addrs: | ||
for txid in blockchain.txsForAddr(addr): |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
It keeps growing. |
There was a problem hiding this 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:
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?
accounts.py
Outdated
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stakeDiff
error fixed.
I'll do |
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. |
Working great! 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
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. Restart wallet. After sync completes. Presumably this is the correct amount. |
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.
pydecred/account.py
Outdated
# 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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Propagate broadcast exceptions.
if cfg.net.Name == "mainnet": | ||
self.pools = [p for p in pools if tNow - p["LastUpdated"] < 86400 and self.scorePool(p) > 95] |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this 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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doubly approved.
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.
The main goal of this effort was to reproduce dcrwallet's
(*Wallet).purchaseTickets
method. The mirrored method is implemented on theDcrdataBlockchain
, 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.