Skip to content
This repository has been archived by the owner on Feb 15, 2024. It is now read-only.

SegWit #104

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

SegWit #104

wants to merge 3 commits into from

Conversation

libbitc
Copy link
Contributor

@libbitc libbitc commented Aug 18, 2017

Hi,

This PR adds SegWit functionality to picocoin.
All json test files in test/data directory copied from bitcoin core 14.2

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 78.767% when pulling 3da9804 on libbitc:picocoin into faa9b83 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 78.767% when pulling 3da9804 on libbitc:picocoin into faa9b83 on jgarzik:master.

@jgarzik
Copy link
Owner

jgarzik commented Aug 19, 2017

Nice! Thanks a bunch. concept ACK

Does the default-off sync test work on segwit or segwit2x testnet chains?

@libbitc
Copy link
Contributor Author

libbitc commented Aug 21, 2017 •
edited
Loading

Hi,

SegWit aside the default-off chain-verf test does not pass for testnet3 blocks:

chain-verf: validating testnet3 chainfile /bitcoin/bootstrap.testnet3.dat (*script)
chain-verf: spend block @ 0
chain-verf: spend block @ 5000
chain-verf: spend block @ 10000
chain-verf: spend block @ 15000
chain-verf: tx fail 970b18f67be0b86b8994223a87ef445431e5efc03fe2191034e9dad0c17dd6d3
chain-verf: block fail 16306 000000001531abe940a7983079579fa4e28217ec854b0189d184de744e475b88
chain-verf: chain-verf.c:164: read_test_msg: Assertion `!"spend_block"' failed.
FAIL chain-verf (exit status: 134)

This problem was introduced with changes made in commit 5475db7.

Will add some fixes for this.

@libbitc libbitc force-pushed the picocoin branch 2 times, most recently from 259c42e to 24097bb Compare August 21, 2017 01:23
@libbitc libbitc changed the title SegWit WIP: SegWit Aug 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 78.767% when pulling 24097bb on libbitc:picocoin into faa9b83 on jgarzik:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 78.767% when pulling 24097bb on libbitc:picocoin into faa9b83 on jgarzik:master.

@coveralls
Copy link

coveralls commented Aug 27, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 78.733% when pulling 7787e4c on libbitc:picocoin into faa9b83 on jgarzik:master.

@libbitc
Copy link
Contributor Author

libbitc commented Sep 2, 2017

Hi,

The most recent commit to this PR successfully validates 1179472 blocks of the testnet3 chain.
The block time of block 1179472 is 2017-08-20 03:31:53. testnet3 supports SegWit as of block 834624.(2016-05-13 00:22:51)

chain-verf: validating testnet3 chainfile /libbitc/test/data/bootstrap.testnet3.dat.1179500 (+script)
chain-verf: spend block @ 0
chain-verf: spend block @ 5000
chain-verf: spend block @ 10000
chain-verf: spend block @ 15000
chain-verf: spend block @ 20000
chain-verf: spend block @ 25000
chain-verf: spend block @ 30000
.
.
.
chain-verf: spend block @ 80000
chain-verf: spend block @ 85000
chain-verf: spend block @ 90000
chain-verf: spend block @ 95000
chain-verf: spend block @ 100000
.
.
.
chain-verf: spend block @ 1150000
chain-verf: spend block @ 1155000
chain-verf: spend block @ 1160000
chain-verf: spend block @ 1165000
chain-verf: spend block @ 1170000
chain-verf: spend block @ 1175000
chain-verf: 1179472 records validated
PASS chain-verf (exit status: 0)

Test performed with FORCE_SCRIPT_VERF=1.

@libbitc libbitc changed the title WIP: SegWit SegWit Sep 2, 2017
jgarzik
jgarzik reviewed Sep 2, 2017
View reviewed changes
lib/script_names.c Outdated

for (i = 0; i < ARRAY_SIZE(opnames); i++) {
for (i = 0; i < ARRAY_SIZE(opnames); i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent changes

lib/script_sign.c Outdated

/* match fromPubKey against templates, to find what pubkey[hashes]
/* match fromPubKey against templates, to find what pubkey[hashes]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

test/Makefile.am Outdated
@@ -10,9 +10,9 @@ AM_CPPFLAGS = -DTEST_SRCDIR=\"$(top_srcdir)/test\" \
-I$(top_srcdir)/include \
-I$(top_srcdir)/external/secp256k1/include

noinst_LIBRARIES= libtest.a
check_LTLIBRARIES = libtest.la

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a noinst lib. Why change to libtool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the check_ prefix did not work with the LIBRARIES primary but I was mistaken. Reading the automake manual I now see that:

The special prefix ‘check_’ indicates that the objects in question should not be built until the ‘make check’ command is run. Those objects are not installed either.

The current primary names are ‘PROGRAMS’, ‘LIBRARIES’, ‘LTLIBRARIES’, ‘LISP’, ‘PYTHON’, ‘JAVA’, ‘SCRIPTS’, ‘DATA’, ‘HEADERS’, ‘MANS’, and ‘TEXINFOS’.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, check_ prefix has slightly negative value versus noinst_, because it reduces the amount of code built by default, which prevents immediate notification of test breakage caused by a non-test code bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit reverts check_ prefix to noinst_

test/script.c

#include "picocoin-config.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was dropped. Not needed?

Copy link
Contributor Author

@libbitc libbitc Sep 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running test/script.c through iwyu tool would suggest so.

test/script.c Outdated
const char* scriptSigString,
const char* scriptPubKeyString,
const unsigned int test_flags,
int64_t nValue)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: non-standard indent versus rest of project.

@libbitc
Copy link
Contributor Author

libbitc commented Sep 2, 2017

Hi,

Non-standard indentations noted. .clang-format config seems to be doing some strange things. Will fix.

@coveralls
Copy link

coveralls commented Sep 2, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.9%) to 77.642% when pulling 8a454a9 on libbitc:picocoin into faa9b83 on jgarzik:master.

@coveralls
Copy link

coveralls commented Sep 2, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.9%) to 77.642% when pulling 8fbaf2c on libbitc:picocoin into faa9b83 on jgarzik:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jgarzik jgarzik jgarzik left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@libbitc @coveralls @jgarzik