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

Segwit #1334

Merged
merged 9 commits into from Jun 29, 2017
Merged

Segwit #1334

merged 9 commits into from Jun 29, 2017

Conversation

@xenog
Copy link
Contributor

xenog commented Jan 20, 2017

Support for segwit. Please review thoroughly. Comments and modifications strongly encouraged.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jan 20, 2017

We can probably skip the "segnet" and "segwit addresses" commits - since testnet3 now has segwit no need for segnet, and the BIP for segwit addresses was withdrawn pending future work in the space.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 30, 2017

Coverage Status

Coverage decreased (-0.003%) to 72.272% when pulling 7697829 on xenog:segwit into 380abd0 on bitcoinj:master.

Copy link
Member

schildbach left a comment

Thanks, this is a great contribution!

I've added some in-line review comments, but it's all rather minor. I agree with @TheBlueMatt that segnet is probably not useful anymore. I'd keep segwit addresses for now, as long as there is no better proposal.

We develop segwit on a branch rather than master. This is because it's questionable if it ever activates. Can you rebase your PR on that segwit branch? It contains already some code by @NicolasDorier – I'm not sure if it makes sense to keep but just in case I rebased the existing code on master.

If you feel some subset of your contribution makes sense on current master – even with segwit not activating –, feel free to submit it in a separate PR against master. Same goes for BIP9 support which I think we should implement before segwit.

@@ -1451,6 +1451,7 @@ private void blockChainDownloadLocked(Sha256Hash toHash) {
this, toHash, chainHead.getHeader().getHashAsString());
StoredBlock cursor = chainHead;
for (int i = 100; cursor != null && i > 0; i--) {
//

This comment has been minimized.

Copy link
@schildbach

schildbach Feb 1, 2017

Member

Superfluous comment

@@ -39,6 +39,8 @@
private Sha256Hash hash;
/** Which output of that transaction we are talking about. */
private long index;
/** Value of previous output. */

This comment has been minimized.

Copy link
@schildbach

schildbach Feb 1, 2017

Member

Trying to understand why you add the value here. Did you realize it's already in TransactionInput? (optional, only if the input is connected)

return empty;
}

byte[][] pushes;

This comment has been minimized.

Copy link
@schildbach

schildbach Feb 1, 2017

Member

Private perhaps?

@@ -0,0 +1,29 @@
/*
* Copyright 2012 Google Inc.

This comment has been minimized.

Copy link
@schildbach

schildbach Feb 1, 2017

Member

Please update the copyright line after cut & paste of the license header. It's fine to use "Copyright 2017 the bitcoinj authors" if you don't care much.

@@ -35,6 +34,8 @@
* and testing of applications and new Bitcoin versions.
*/
public class TestNet3Params extends AbstractBitcoinNetParams {
public static final int SEGWIT_ENFORCE_HEIGHT = 834624; // TODO: implement BIP-9 instead

This comment has been minimized.

Copy link
@schildbach

schildbach Feb 1, 2017

Member

Yes, I think BIP9 should be implemented ahead of this PR.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Feb 1, 2017

@schildbach Please, please, do not encourage people to add bitcoinj-specific addresses for transaction types. There isnt any rush to have addresses if no one else will support them - skip it for now and wait a month or two until there are other proposals and the community can have a reasonable discussion.

@schildbach

This comment has been minimized.

Copy link
Member

schildbach commented Feb 1, 2017

All I wanted to say is: I see no need to remove that implementation that has alredy been written. Don't worry, we'll set no precedent here: I do not intend to merge unless it's clear segwit activates, so there is plenty of time for more proposals/implementations.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Feb 1, 2017

Mmm, ok, I mean I would still remove the address format so that it is ready to merge if segwit were to activate (as it would still otherwise set precedent), but, I suppose its a moot point unless segwit gets much closer.

@rktoomey

This comment has been minimized.

Copy link

rktoomey commented Feb 3, 2017

It would be helpful to have this available as soon as possible because right now bitcoinj is not working with testnet3 on bitcoind 0.13+ nodes - see 0000000000332ef392f66e80afa2bb1389c08f47050ea189ae432e7bfea8c36e for an example of a block that fails to successfully deserialize. .

@schildbach

This comment has been minimized.

Copy link
Member

schildbach commented Feb 3, 2017

@rktoomey Segwit is a softfork, so if current bitcoinj fails with a segwit block, that's a serious bug. Can you open a separate issue and attach the logfile?

@schildbach

This comment has been minimized.

Copy link
Member

schildbach commented Feb 3, 2017

@rktoomey Is your app using a bloom filter? I had a look at my Bitcoin Wallet logfile and it had no trouble synching past the block. But that's expected, as it hardly sees any transactions due to the filter.

@rktoomey

This comment has been minimized.

Copy link

rktoomey commented Feb 3, 2017

@schildbach I'm filing an issue with exact details - there are multiple factors. Users who are using pre 0.13 nodes should be unaffected.

@Polve

This comment has been minimized.

Copy link

Polve commented Jun 23, 2017

It seems like in a way or another we are getting segwit: could this be merged?

@xenog

This comment has been minimized.

Copy link
Contributor Author

xenog commented Jun 23, 2017

I'm doing some cleanup at the moment, merging latest master in, removing Base58 SegWit addresses, looking at block validation and P2P support. I'll keep you posted.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-0.05%) to 72.357% when pulling 220a64e on xenog:segwit into 0eabd24 on bitcoinj:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.001%) to 72.41% when pulling 1b7e12e on xenog:segwit into 0eabd24 on bitcoinj:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.06%) to 72.464% when pulling 038faa2 on xenog:segwit into 0eabd24 on bitcoinj:master.

@xenog xenog changed the base branch from master to segwit Jun 26, 2017
@xenog

This comment has been minimized.

Copy link
Contributor Author

xenog commented Jun 26, 2017

I changed base of this PR to bitcoinj:segwit instead of bitcoinj:master as recommended.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.02%) to 72.454% when pulling 5eca2ae on xenog:segwit into d50612d on bitcoinj:segwit.

@xenog xenog closed this Jun 26, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.02%) to 72.45% when pulling d50612d on xenog:segwit into d50612d on bitcoinj:segwit.

@xenog

This comment has been minimized.

Copy link
Contributor Author

xenog commented Jun 26, 2017

GitHub went bananas when I rebased.

@xenog xenog reopened this Jun 27, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-0.2%) to 72.271% when pulling 9a30180 on xenog:segwit into d50612d on bitcoinj:segwit.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-0.2%) to 72.224% when pulling 9a30180 on xenog:segwit into d50612d on bitcoinj:segwit.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.2%) to 72.276% when pulling 3553d31 on xenog:segwit into d50612d on bitcoinj:segwit.

@luke-jr

This comment has been minimized.

Copy link

luke-jr commented Jun 28, 2017

@xenog Are you working on versionbits support?

@xenog

This comment has been minimized.

Copy link
Contributor Author

xenog commented Jun 28, 2017

@luke-jr if you mean BIP-9 support, I haven't started yet. Want to help?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage decreased (-0.2%) to 72.272% when pulling 246983c on xenog:segwit into d50612d on bitcoinj:segwit.

@schildbach schildbach merged commit 6f41f30 into bitcoinj:segwit Jun 29, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -1,50 +0,0 @@
/*

This comment has been minimized.

Copy link
@schildbach

schildbach Jun 29, 2017

Member

You removed the example again, was this intentional?

@schildbach

This comment has been minimized.

Copy link
Member

schildbach commented Jun 29, 2017

Merged this to segwit. Code style looks very good, and thanks for all the tests! I didn't review all the logic yet, guess I have to learn a lot about segwit (-:

If you see on the segwit branch a refactoring that would benefit bitcoinj already now, and works towards merging the branch, don't hesitate to mention this because merging and testing "preparational" changes early helps kepping the impact of merging the segwit feature low.

@schildbach

This comment has been minimized.

Copy link
Member

schildbach commented Jun 29, 2017

Yes, if anyone wants to tackle BIP9 (on a separate PR), that'll be great. It's a prerequisite for proper segwit support. Also, I think Bech32/segwit addresses hasn't been implemented yet (I might do this myself if no one objects).

@xenog

This comment has been minimized.

Copy link
Contributor Author

xenog commented Jun 29, 2017

No objections here. I am working on consensus code at the moment. On my roadmap for the next week or two:

  • Counting segwit sigops
  • Verifying witness structure in coinbase when segwit is active
  • Peer-to-peer support for moving segwit data
@ghost ghost mentioned this pull request Feb 17, 2018
@tiger1342

This comment has been minimized.

Copy link

tiger1342 commented Jul 10, 2018

Hello guy,

I am trying to use Segwit in BitcoinJ but didn't find anything about it in API. please let me know if it was ever committed into Master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.