-
Notifications
You must be signed in to change notification settings - Fork 26
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
Enhancement: Tx Unlocker #67
Conversation
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 84.75% 85.09% +0.33%
==========================================
Files 28 28
Lines 3011 3012 +1
==========================================
+ Hits 2552 2563 +11
+ Misses 325 313 -12
- Partials 134 136 +2
Continue to review full report at Codecov.
|
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.
LGTM, just a really tiny thing I found
@@ -195,17 +195,15 @@ func (tx *Tx) CalcInputPreimageLegacy(inputNumber uint32, shf sighash.Flag) ([]b | |||
} | |||
} | |||
|
|||
switch shf & sighash.Mask { // nolint:exhaustive // no need | |||
case sighash.None: | |||
if shf.HasWithMask(sighash.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.
nice
localunlocker_test.go
Outdated
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestInternalSigner_SignAll(t *testing.T) { | ||
func TestLocalSignatureUnlocker_UnlockAll(t *testing.T) { |
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 think you forgot this
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.
yep, fixed here:
# tigh in ~/go/src/github.com/libsv/go-bt on git:enhancement/tx-unlocker o
> grep -rsn LocalSignatureUnlocker
# tigh in ~/go/src/github.com/libsv/go-bt on git:enhancement/tx-unlocker o
@Tigh-Gherr I'm realising that I really don't like the double negative here with the "unlocking" especially where we have txs which are not yet unlocked and then we settled for "incomplete transactions". I think we should take a step back and revert to something similar to what we had before. "Signing" and "unsigned" txs are not accurate anymore after genesis since the locking/unlocking scripts are not restricted to only p2pkh scripts. However, what if we change it to "signoff" and "unsignedoff" txs so it's still similar but instead of signing a tx you would be signing it off and that makes sense in the general form. I think it's much better than the double negative with "unlocking" and "incomplete" txs that was pretty confusing. What do you think? @Tigh-Gherr |
@jadwahab we are talking about the fund itself being locked/unlocked, not the tx. So we take a locked UTXO, provide the script to unlock it, and boom, we've all the money. So we unlock a locked fund. The "incomplete" term comes exclusively from test code, there isn't a public interface anywhere in go-bt (that I know off) that names it such. The closest public interface we have to performing this behaviour is the Fund function. We fund an unfunded tx, then we unlock those funds. In my opinion, a locked fund that we unlock paints a much clearer image over a fund that isn't signed off, which we then sign off. Especially because signing off does imply a signature. Also, "unlock" is shorter to type than "sign off". |
Hahahaha gtfoh mate @Tigh-Gherr Yeah I agree with you the the fund is locked and unlocked but I was talking about the tx inputs. So basically where we have tx.Unlock (which is tx.Sign in v1) for each input in the tx. We used to have a concept of an "unsigned" tx where the inputs are there but not signed/unlocked - this is where the double negative becomes confusing and why we had to resort to using the "incomplete tx" in the tests (we will also use this term in other parts of the code since "unsigned" txs are used in so many places). So in order to avoid saying the "not unlocked" tx (or the "locked tx" which sounds really weird - UTXOs are locked but transactions don't really make sense to be locked in this context and can easily be confused for txs locked with nLockTime) - I think it would be better to use "unsignedoff" inputs and transactions. That's also closer to v1 and legacy terminology where they used to be "unsigned" inputs and txs. TLDR: "unsignedoff" is shorter to type than "not unlocked" 😅 |
Aw shit @jadwahab I think we're at a stalemate:
If the worry is around the My issue about reverting back is that, we'd be regressing on the terminology at the fund level just to make it a bit friendlier at the level of a tx (and being honest, I'm sceptical that this makes it friendlier at the level of a tx). It's a common analogy that's really pervasive throughout programming, and something we meet every single day of our lives in real life. It's a much more familiar than the idea of a signed off/unsigned off entity. A locked/unlocked entity also paints a far clearer picture of both the state of the current fund/tx, and is generally more applicable to the language around the possible uses of locking/unlocking scripts. To bring it into terms of the travelling salesman problem, it's more intiuative to say you unlock the fund with a solution to the problem, as opposed to saying you signed off on the fund with a solution to the problem. In regards to the implementation of this, we'd have to consider our interface and struct implementation names:
|
Yupp we can keep those as Unlocker since they apply to the locking script directly. And then have |
The current signer is restrictive in both language an implementation, as it assumes only checksig based unlocking will be used.
To sort this, I've added an
Unlocker
interface andtx.Unlock
functions which can take any type of unlocking implementation, and provided our p2pkh signing implementation out of the box as aLocalP2PKHUnlocker
.