staking: Add revocations #30
Conversation
|
I really wish "draft" was the default. |
There was a problem hiding this comment.
This is looking good. I'll treat this as in draft until you remove WIP, but I left a couple comments for now.
The database concurrency error is annoying. You'll need to track down whatever query is causing it, and make sure to create a new connection. Note that if the db is used contextually, such as in DcrdataBlockchain.tx, a new connection is created by default.
The database issues will be smoothed out in the upcoming db work.
ui/screens.py
Outdated
| revokeBtn = app.getButton(TINY, "Revoke") | ||
| revokeBtn.clicked.connect(self.revokeTickets) | ||
| votingWgt, _ = Q.makeSeries(Q.HORIZONTAL, agendaBtn, revokeBtn) | ||
| self.layout.addWidget(votingWgt) |
There was a problem hiding this comment.
Consider adding a QLabel that says "no tickets to revoke", then hiding the label and showing the button only after checking the revocation count on SYNC_SIGNAL.
There was a problem hiding this comment.
Hiding the button anyway. Do we need to show anything when there's nothing to revoke?
JoeGruffins
force-pushed
the
revocations
branch
3 times, most recently
from
47cfe1a
to
17185bf
Compare
|
ok, let me have it |
There was a problem hiding this comment.
Mostly just comments about raising exceptions and some typos. A couple substantive questions though.
Most of the functions you've translated didn't have explicit testing in dcrd, but one test you could implement is TestSignatures from dcrd/dcrec/secp256k1. Maybe take a peek around dcrd and see if there are others.
pydecred/account.py
Outdated
| else: | ||
| log.error("did not find redeem script for hash %s" % redeemHash) | ||
| errored = True | ||
| continue |
There was a problem hiding this comment.
Indentation is wrong here. I really thought black would have caught this.
There was a problem hiding this comment.
Raising an exception now. But not sure about the indentation.
There was a problem hiding this comment.
Oh. I was reading that wrong. Indentation is correct.
There was a problem hiding this comment.
You could do this cleanly in two lines
redeemScript = next((decodeBA(p.purchaseInfo.script) for p in self.stakePools if p.purchaseInfo.ticketAddress == redeemHash.string()), None)
assert redeemScript is not None
although that first line is a little tedious. for ... else is weird to see in the wild.
Add a button to enable revocation of all missed and expired tickets.
- Add basic dcrdata revoke ticket test. - Add documentation. - Format with black. - Move txscript functions around.
Also add more parsing checks for signature. Raise more exceptions. Log how many tickets revoked upon successful revocation.
JoeGruffins
force-pushed
the
revocations
branch
from
4fc8b91
to
6b68f6a
Compare
|
Still need to look into more tests. |
|
Added TestSignatures. Wasn't sure of the best place to put the test so just made a new file. Although it's in pydecred as part of txscript, perhaps it belongs in crypto? |
JoeGruffins
force-pushed
the
revocations
branch
from
51fba21
to
8ba10e7
Compare
| @@ -0,0 +1,290 @@ | |||
| from tinydecred.crypto.bytearray import ByteArray | |||
| from tinydecred.pydecred.txscript import Signature | |||
There was a problem hiding this comment.
Forgot the copyright thing
There was a problem hiding this comment.
Looking good. A couple of last things.
Please either lower-case the voting button or upper-case the revoke button.
See dcrd's TestSignTxOutput for some tests that might be relevant. I've already started that test function, but it doesn't cover all of TD's capabilities anymore. Looks like I should have added some there back in #7 too.
|
changed voting to lower case. I add two tests to the sign tx output and started to add basic multisig but stopped when I realized we also need script-hash signing to test the same as dcrd. Moar tests? |
Add tests for signing txoutput. Not complete. Add ability to create multisig scripts.
JoeGruffins
force-pushed
the
revocations
branch
from
d772571
to
5578dba
Compare
staking: Add revocations. Additional multi-sig script support. Add signature test coverage. UI: Add a button to enable revocation of all missed and expired tickets.
closes #29
Add a button to enable revocation of all missed and expired tickets.
Use a lot of code from dcrwallet and dcrd to revoke tickets. It's still very rough so don't look too closely, but it's working.
There is one thing I would like @buck54321 to comment on, I get this message sometimes when revoking multiple tickets:
2019-12-20 10:58:42,411 account ERROR revokeTickets(389) error getting tx: SQLite objects created in a thread can only be used in that same thread. The object was created in thread id 139999400998656 and this is thread id 139999383426816.
I understand the general problem that it's a concurrency issue, but I'm not quite sure of the cause or the best way to handle it. Thanks!