-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Can serialize and deserialize transaction with witnesses #1171
Can serialize and deserialize transaction with witnesses #1171
Conversation
You can cross check transactions on https://blockchainprogramming.azurewebsites.net/checktx There is also another problem which I bumped into with NBitcoin: It is impossible to deserialize which those 2 conditions : No inputs, One or More Output. In practice this is not a big deal because transaction without input does not exist... But sometimes, during internal processing, you may serialize and deserialize for cloning your transaction for example. And I've encountered case in NBitcoin where it happened without input. The way I dealt with it is by fitting a bit in the tx version which means "I really have no witness, don't read the next byte as flag". I have not implemented that here. |
Thanks, that's great! I think this PR misses the code for protobuf-serialization of witnesses. So a wallet that saves to disk and back will miss the witnesses. Also please watch the code style. Though I can fix that while merging. |
byte[] bits = bitcoinSerialize(); | ||
ByteArrayOutputStream stream = new UnsafeByteArrayOutputStream(length < 32 ? 32 : length + 32); | ||
try{ | ||
bitcoinSerializeToStream(stream, TransactionOptions.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.
indentation
I tried to be careful with code style did I missed something ? |
} | ||
|
||
private void readWitness() | ||
{ |
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.
'{' at end of line
Fixing formatting, I did not knew how to do it on android studio :p |
a32d924
to
4386a9d
Compare
@schildbach ok I reformatted, some other things has been catched up that were not properly formatted. |
@schildbach Actually, there is a second way of doing, which I did with NBitcoin. In this PR, each transaction have "outputs", "inputs" and "witnesses" where witnesses is either null OR of exactly the size of inputs. It matches the serialization model on a bitcoin transaction. (witnesses are not serialized in the "TxInput"). In NBitcoin I initially took this approach, but found it bothersome to develop against. So I changed and added an accessor "Witness" into the "TxInput" class. This does not match the serialization model, however, it is conceptually more correct. |
Protobuf serialization is done in the WalletProtobufSerializer class and the structure will be extended in wallet.proto. Don't forget to generate fresh bindings – see comment in wallet.proto. It's fine to do it in a separate commit. Separate PR feels weird though, at least it depends on your changes already done. About the witnesses structure I have no preference right now. Usually it makes sense to look at how Bitcoin Core does things. Also, maybe it makes the decision easier if we anticipate there can be multiple witnesses in future. If you use a formatter, please make sure to format only your code. Correcting already existing format problems should go into a different PR (don't bother about it now). It may sound a bit finicky, but keeping changes to the minimum helps with reviewing and backporting your code. If you're unsure with formatting, I can do it at merge time. |
|
||
public class TransactionWitness { | ||
public TransactionWitness(int pushCount) | ||
{ |
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.
'{' to the EOL please
9b2c6b1
to
d12e856
Compare
Ok now it is perfectly formatted, I'll be more careful next commit. There is other things to do for segwit, one of which is the new hash function for signature, do I do that in the same PR as well ? |
d12e856
to
63217e6
Compare
@@ -278,6 +278,33 @@ public void testCLTVPaymentChannelTransactionRefund() { | |||
} | |||
|
|||
@Test | |||
public void canParseTransaction() { |
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.
Maybe rename this test to something more meaningful, e.g. witnessTransaction()
?
The formatting looks very good now. Thanks for making the effort. You're right about witnesses not needed by the wallet (unless there will be future witnesses that are useful), so yes let's skip protobuf persistence for now. Yes if you have further work for segwit feel free to put a commit on top. Ideally we'd work towards a usable feature (like those I pictured in my "rough roadmap" posted to the mailing list) and if one is finished, and the BIPs are finished/committed to by Bitcoin Core, we merge. |
63217e6
to
7aa4ead
Compare
Can you advise also what you would prefer: Currently if you serialize then deserialize a transaction with no input BUT with output, it will fail, due to how segwit format has been designed. My fix to that was to put a flag in the version during serialization if those 3 conditions were true : Segwit supported, Input size is 0, Outputs size is above 0. |
I prefer to do without that flag workaround. And you said this PR comes without – still it passes all the unit tests so I guess it's no problem for bitcoinj. |
dbf6b45
to
05fa6ce
Compare
New hashSignature scheme implemented, I borrowed from @greenaddress greenaddress@891e349 |
5437939
to
f708088
Compare
@schildbach I think I finished all is needed for basic segwit support. To my mind there is still some nice to have method to make, like a way to go back and forth between Script and TransactionWitness class. (A TransactionWitness is basically a Script with only pushes encoded more efficiently) Let me know if you would like something more or if you'd like any fix. (or if you have any question on segwit) |
@schildbach it can be useful to merge that before it get merged to core so people can start testing on segnet. Let me know if you want I add something in this PR. |
I'd rather wait with merging to master until the spec is committed (will most likely not change any more). I can merge it to a branch on our repo, but then again this is only useful if we have multiple developers working on segregated witness. |
Ok no problem, I'll squash the commits as you said, and I will also add the segnet params. (I forgot about that) |
f708088
to
75169e2
Compare
Ok @schildbach segnet is now pushed and the commit you mentioned squashed. Ping @greenaddress I know you need that for your tests. |
8ede6f7
to
ea1596f
Compare
Added ref to the BIP, as well as the BIP's sample in tests |
We tested on segnet3 this patch as our code doesn't use the rest. what is the plan for this PR? I believe the spec is not changing anymore. @NicolasDorier it probably needs rebase (and maybe squash) :) |
The plan is to let SegWit mature (as a whole, so also the work on Core). As soon as Core SegWit merged, I'd merge all parts of this PR that form a viable usecase. Until then, if multiple people start contributing code to this PR I can merge it to a feature branch. |
sounds good, thanks @schildbach! |
ea1596f
to
912d7e4
Compare
rebased |
Changes Unknown when pulling 912d7e4 on NicolasDorier:can-parse-transaction-with-witness into * on bitcoinj:master*. |
Thanks a lot for starting to work on the heavy lifting required to utilise SegWit! I started using this branch for (thunder)[https://github.com/blockchain/thunder/commit/e9132c9f30719f430385e88cc1c04d7611b5dfd0], and added a few utility methods in my code that probably rather belong over here (like constructing a TransactionWitness object out of a P2SH output and input). I think a feature branch makes sense at this point |
Feature branch done: https://github.com/bitcoinj/bitcoinj/tree/segwit @matsjj Great to know you're working on thunder/lightning! |
A note abotu segwit, the witness program size was extended to 40 bytes in BIP141 c.f. bitcoin/bips@d1b52cb |
First step for implementation of segwit, transaction serialization and deserialization