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.

Sign up for GitHub

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

Jump to bottom

Segwit Compatibility #47

Closed
wants to merge 19 commits into from
Closed

Segwit Compatibility #47

wants to merge 19 commits into from

Conversation

achow101
Copy link

This PR should be everything that is needed in order to be compatible with segwit. This means that Armory will still be able to work with the changes to block and transaction formats. However it does not actually use segwit yet; that is the next step.

This PR also merges #22 for the regtest mode. This was done in order to test the segwit compatibility in an isolated environment where I could control everything.

@achow101 achow101 force-pushed the segwit-dev-rebase branch 2 times, most recently from 235994b to 01a9fdb Compare July 23, 2016 02:20
@achow101 achow101 mentioned this pull request Jul 23, 2016
Closed
@achow101
Copy link
Author

I did not write unit tests for this since I don't know how. I can however provide segwit transactions and blocks for unit tests if you need them.

To test to see if this works by hand, you will need to have Bitcoin Core 0.13+ (Either use an rc release or build from the repo). Then start both Bitcoin Core and Armory in regtest mode. In Bitcoin Core, go to the console and generate 432 blocks. That should activate segwit (check by using getblockchaininfo). Then you can test all of the different sending scenarios. Witness addresses can be made using addwitnessaddress in Bitcoin Core.

achow101
achow101 reviewed Jul 24, 2016
View reviewed changes
cppForSwig/BtcUtils.h
return TXOUT_SCRIPT_NONSTANDARD;
else if (sz == 21 &&
s[0] == 0x00)
return TXOUT_SCRIPT_P2WPKH;
Copy link
Author

Choose a reason for hiding this comment

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

Needs to check the size of the rest of the script data. Check for 0x14 byte

goatpig
goatpig reviewed Jul 24, 2016
View reviewed changes
cppForSwig/BtcUtils.h
witLen += viStackLen;
for(uint32_t i=0; i<stackLen; i++)
{
uint32_t viLen;
Copy link
Owner

Choose a reason for hiding this comment

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

should check there's at least 1 byte left to grab a varint here (per iteration)

goatpig
goatpig reviewed Jul 24, 2016
View reviewed changes
cppForSwig/TxClasses.cpp
lockTime_ = READ_UINT32_LE(ptr + offsetsTxOut_[numTxOut]);
uint32_t numTxOut = offsetsTxOut_.size() - 1;
version_ = READ_UINT32_LE(ptr);
if (4 > size - offsetsTxOut_[numTxOut])
Copy link
Owner

Choose a reason for hiding this comment

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

this check doesn't work anymore, you need to check against the end of the witness data. this means you need to set offsetWitness_ in all cases (with or without witness data), and check
(4 > size - offsetsWitness_[numWitness])
where numWitness is
offsetWitness_.size() - 1;

This implies offsetWitness is set to offsetsTxOut[numTxOut] where there is no witness data.

goatpig
goatpig reviewed Jul 24, 2016
View reviewed changes
armoryengine/Transaction.py
if stackSize > 0:
for i in range(0, stackSize, 1):
stackItemSize = txWitnessData.get(VAR_INT)
debugremsize = txWitnessData.getRemainingSize()
Copy link
Owner

Choose a reason for hiding this comment

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

unused local variable

Copy link
Author

Choose a reason for hiding this comment

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

oops. that was for debugging

goatpig
goatpig reviewed Jul 24, 2016
View reviewed changes
armoryengine/Transaction.py
indstr2 = indstr + indent
result = indstr + 'PyWitness:'
result = ''.join([result, '\n', indstr2 + 'Stack Size:', \
str(self.binWitnesses[0])])
Copy link
Owner

Choose a reason for hiding this comment

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

where is self.binWitnesses defined? I only see self.binWitness

goatpig
goatpig reviewed Jul 24, 2016
View reviewed changes
armoryengine/Transaction.py
self.version = txData.get(UINT32)
marker = txData.get(UINT8)
flag = txData.get(UINT8)
if marker == MARKER and flag == FLAG:
Copy link
Owner

Choose a reason for hiding this comment

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

maybe rename these as WITNESS_MARKER and WITNESS_FLAG across the board, it will be confusing a year down the road when no one is thinking about segwit when reviewing code

@achow101 achow101 force-pushed the segwit-dev-rebase branch 4 times, most recently from dedb731 to 6f7987f Compare July 26, 2016 20:35
@achow101
Copy link
Author

If I run ArmoryDB and ArmoryQt separately, I get a few errors on each network.

On regtest, it segfaults due to a buffer overflow somewhere.

On testnet, I get

(ERROR) Traceback (most recent call last):
  File "ArmoryQt.py", line 6688, in method_signal
    method()
  File "ArmoryQt.py", line 6728, in completeBlockchainProcessingInitialization
    self.setupLedgerViews()
  File "ArmoryQt.py", line 6743, in setupLedgerViews
    self.ledgerModel.setLedgerDelegate(TheBDM.bdv().getLedgerDelegateForWallets())
  File "/home/andy/bitcoin/BitcoinArmory/CppBlockUtils.py", line 1261, in getLedgerDelegateForWallets
    def getLedgerDelegateForWallets(self): return _CppBlockUtils.BlockDataViewer_getLedgerDelegateForWallets(self)
DbErrorMsg: <CppBlockUtils.DbErrorMsg; proxy of <Swig Object of type 'DbErrorMsg *' at 0x7fa7dbfd1c90> >

Traceback (most recent call last):
  File "ArmoryQt.py", line 6688, in method_signal
    method()   
  File "ArmoryQt.py", line 6728, in completeBlockchainProcessingInitialization
    self.setupLedgerViews()
  File "ArmoryQt.py", line 6743, in setupLedgerViews
    self.ledgerModel.setLedgerDelegate(TheBDM.bdv().getLedgerDelegateForWallets())
  File "/home/andy/bitcoin/BitcoinArmory/CppBlockUtils.py", line 1261, in getLedgerDelegateForWallets
    def getLedgerDelegateForWallets(self): return _CppBlockUtils.BlockDataViewer_getLedgerDelegateForWallets(self)
<class 'CppBlockUtils.DbErrorMsg'>: <CppBlockUtils.DbErrorMsg; proxy of <Swig Object of type 'DbErrorMsg *' at 0x7fa7dbfd1c90> >

But when I ran it again, it worked

And on mainnet I get the same thing as testnet and a dialog saying something about the wrong magic bytes were detected.

Running just ArmoryQt.py without the db already running gets the below for mainnet and regtest

(ERROR) Traceback (most recent call last):
  File "ArmoryQt.py", line 6688, in method_signal
    method()
  File "ArmoryQt.py", line 6728, in completeBlockchainProcessingInitialization
    self.setupLedgerViews()
  File "ArmoryQt.py", line 6743, in setupLedgerViews
    self.ledgerModel.setLedgerDelegate(TheBDM.bdv().getLedgerDelegateForWallets())
  File "/home/andy/bitcoin/BitcoinArmory/CppBlockUtils.py", line 1261, in getLedgerDelegateForWallets
    def getLedgerDelegateForWallets(self): return _CppBlockUtils.BlockDataViewer_getLedgerDelegateForWallets(self)
DbErrorMsg: <CppBlockUtils.DbErrorMsg; proxy of <Swig Object of type 'DbErrorMsg *' at 0x7f3656209c90> >

Traceback (most recent call last):
  File "ArmoryQt.py", line 6688, in method_signal
    method()   
  File "ArmoryQt.py", line 6728, in completeBlockchainProcessingInitialization
    self.setupLedgerViews()
  File "ArmoryQt.py", line 6743, in setupLedgerViews
    self.ledgerModel.setLedgerDelegate(TheBDM.bdv().getLedgerDelegateForWallets())
  File "/home/andy/bitcoin/BitcoinArmory/CppBlockUtils.py", line 1261, in getLedgerDelegateForWallets
    def getLedgerDelegateForWallets(self): return _CppBlockUtils.BlockDataViewer_getLedgerDelegateForWallets(self)
<class 'CppBlockUtils.DbErrorMsg'>: <CppBlockUtils.DbErrorMsg; proxy of <Swig Object of type 'DbErrorMsg *' at 0x7f3656209c90> >

There was also a dialog about invalid magic word.

Testnet worked.

@achow101
Witness negotiation with NODE_WITNESS service bit
1869e63 
Adds negotiation of witness usage through NODE_WITNESS service bit. Adds the new inv types in order to get witness serialization. Also bumps the protocol version and added additional network stuff for handling the new protocol version
@achow101
add segwit input and output types
e661eac 
Add recognization of the new input and output types used by segwit
@achow101
Some transaction witness serialization
9579e64 
Added transaction witness serialization stuff to PyTx.
@achow101
Bunch of fixes; some more transaction serialization
70e1450
@achow101
Fixed block file parsing; more transaction serialization
3a44141 
Block file parsing with witnesses is completely fixed.
@achow101
Fixed transaction info box with segwit txes; more tx serialization
2b56813
@achow101
Miscellaneous changes
9a5d361
@achow101
Add a regtest mode
5773829 
Added everything required to function on the Bitcoin Core Regtest network
@achow101
Add regtest to armoryd
788b703
@achow101
Give GUI error when regtest and testnet are both enabled
e01fa58
@achow101
Fixes transaction unserialization issue
24ab2fe 
Fixes the issues that having wrong transaction unserialization caused.
@achow101
Hash the correct parts of a new ZC tx
f24acd2 
If a transaction is in witness serialization, the part of the transaction without the witness is properly extracted to be hashed.
@achow101
Fix transaction hash for db
050e175 
Transaction hash is properly calculated before db stuff happens. Also fixed blank regtest explorer and did some MSG_INV_WITNESS stuff
@achow101
Rebase fixes
7f53ad5
@achow101
Additional sanity checks and fixes
a8983fa 
Additional check for determining a segwit output. Checks that the second byte of the output is the correct size for witness v0 programs.

Checks for remaining buffers when calculating witness length.
@achow101
Rename MARKER and FLAG
6005ec3 
Rename MARKER and FLAG to WITNESS_MARKER and WITNESS_FLAG
@achow101
Attempt to remove copies from BlockDataMap
9482a2c 
Removed copies from BlockDataMap so that noWitData is only calculated if there is a witness.
@achow101
Networking stuff in cpp code; fixes
be9393f 
Adds networking changes for segwit into cpp code.
@achow101
Copy link
Author

merged in a1a6a29

@achow101 achow101 closed this Jul 29, 2016
@achow101 achow101 mentioned this pull request Jul 29, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@achow101 @goatpig