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

Add P2PK transaction support #440

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

Conversation

joeuhren
Copy link
Contributor

The oldest bitcoin transactions used a script format called P2PK (Pay To Pubkey) which require additional decoding to find the vin and vout public key addresses. This commit fixes the "TypeError: Cannot read property '0' of undefined" error when trying to sync bitcoin by adding support for reading addresses from transactions using the P2PK format.

-Adds additional calls to rpc cmds getdescriptorinfo and deriveaddresses which facilitate decoding of the P2PK data

Solves the following issue: #437

@TheHolyRoger
Copy link
Collaborator

@joeuhren Thank you! Will test this on my branch soon or commit if others have tested/approved

@PRCYDev
Copy link

PRCYDev commented Feb 17, 2021

Works great on a chain I used to have to use some workaround code from other Pull Requests to sync - nice work!

@joeuhren
Copy link
Contributor Author

joeuhren commented Feb 18, 2021

That last commit doesn't really belong here. I figured I would submit another small bug fix since the copy/paste address bug is super annoying to me, but I'm not sure how to create a separate pull request. It just got added to the existing one I had open as soon as I committed to my local repo. If there's some way to move it into a separate pull request I think that would be best, but I'm not sure how yet

UPDATE: I rebased my local repo and now the additional bugfix that didn't belong here is gone. I will re-submit that one in another pull request once finished dealing with this one

@TheHolyRoger
Copy link
Collaborator

One issue I've seen is that getdescriptorinfo and deriveaddresses only exist in newer RPCs unfortunately, based off BTC 0.18+

This would need to add checks for those RPC commands to work.

There's also no need to build your own http requests, if you look at the way other RPC commands are executed, they use the bitcoin RPC.

It's worth you speaking to @uaktags as he's been working on a "coin commands" feature which we were hoping to merge to this branch.

@joeuhren
Copy link
Contributor Author

joeuhren commented Mar 3, 2021

I added a new commit to improve upon some of the shortcomings with the original commit.

Apparently I'm not too good with git as I keep making mistakes lol. cryptoes123 is a dummy account of mine which I have no idea how it got committed as that user since I had to enter credentials for my real account. Anyway, the code is still valid so I'm not going to spend more time trying to correct the wrong user.

New rpc command functions were added which support both rpc and http request depending on the value of the use_rpc setting.

I mixed up the logic earlier and was saying that P2PK addresses were being encoded to P2PKH, but it's actually the other way around. P2PK scripts contains the real public key and the P2PKH address that we all know and love is just a hash which needs to be decoded back to the P2PK value whenever used or referenced in the bitcoin wallet. I updated all references of encode to say decode to be a bit more clear about this.

I also tested a couple older bitcoin daemons, bitcoin-0.17.2 and bitcoin-0.11.1. In both cases I can confirm that the getdescriptorinfo and deriveaddresses rpc commands are not present, yet both versions still pull an address out of the earlier blocks anyway. I didn't spend time checking the bitcoin code, but this tells me that those earlier versions must be automatically converting P2PK to P2PKH (?) or else maybe the peers responding to those older versions have their blocks encoded differently than newer bitcoin releases? Either way, older versions seem to not be a problem. Even if they were, when no address is found in a transaction, this code will attempt to encode an address from P2PK into P2PKH and if not successful it will save the tx as having no addresses and move on to the next block/tx without crashing.

@uaktags I'm guessing your "coin commands" feature would change how some of this is structured. Since this is likely to be a bitcoin-only feature, it might make sense to hold off merging this request until coin commands are integrated and then I can make further changes based on how the new command structure will work.

@joeuhren
Copy link
Contributor Author

joeuhren commented Mar 5, 2021

Just a quick note to say I found some time to rebase my local repo again today. I removed the dummy user and re-committed the same changes under my real username. I double-checked and I don't think I made any mistakes this time :p

@uaktags
Copy link
Collaborator

uaktags commented Jul 2, 2021

Hey, sorry for the long delay. I indeed like this. My idea for the coin implementation is similar to how we handle exchanges:

Create a module for your specific coin, within it have it have its own BOOL functions to let us know if we should use them or not, if not use the "default" method, otherwise use the custom on. It's not pretty, infact, it's disgustingly messy, but I think once others can see the idea and get inspired, we can start tackling this is a cleaner modular way. I'm building it out in https://github.com/uaktags/explorer/tree/master-test for now.

As for this pull, i definitely like it, i just haven't tested to see how it plays with others!

@uaktags
Copy link
Collaborator

uaktags commented Jul 16, 2021

Yea, so after re looking over this, I'm probably going to build a custom BTC coin.js file for this to incorporate the changes. Keep it modular that way. I'll do that probably next week and will have a branch ready for testing and acceptance if you'd like @joeuhren.

@joeuhren
Copy link
Contributor Author

@uaktags that all sounds good to me. I'm in vacation mode at the moment and probably won't have tons of time to update this right away, but I'll keep an eye out for your coin module changes and update this PR to work with the new code when I have the time.

uaktags added a commit to uaktags/explorer that referenced this pull request Jul 20, 2021
@uaktags
Copy link
Collaborator

uaktags commented Jul 20, 2021

I can totally understand that, i've been on vacation mode for what....most of the last 3years ha! Still working through the ideas of how to build in modularity, but the theory sounds solid enough. Implementation....not so much.

@2W-12
Copy link

2W-12 commented Nov 16, 2021

After this commit I can see a lot of errors:

55457: 63971567bab83516916f3a443ae92c48303fe840dc210bd9ad87e3ee81106101
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 67f7f46c60972a6cd3a74130fb0091baf26c417e OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 4d6240987e42b03d8969e1a3e9e49e4d5d43c7d5 OP_EQUALVERIFY OP_CHECKSIG
55461: 31106d0ebe87c23b7cdc1b07a535c5f647bcad409cd2f086d5ada4dfba26aaa5
55378: 553ddc4bc32fd8b2f06a2989a4f590d305920b93c308d9857c0323586daf6a44
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
51728: 49b2a1eb6118d095b72a9474b07e8547ae370c6eff397b89709ca22a2da2cbb8
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 bfacaba187a3851b547e1456bcba64d343f6a942 OP_EQUALVERIFY OP_CHECKSIG
55393: 22b1ff2aa9025f49d686eafc754e8d04491a1b267ada8aa87c266a136cacc5b6
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 3c819a9aa4a671e4dadd635abadeeb3124c7d6d3 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 b88280ca89ff5252753ac015016c4118ca72755d OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 6e0c5203cbf34ea8e5e578efd3fc3a23a10f27c0 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 05bf3a3aea6335a3949c0a351ff3afcba884e125 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 546c51022d302eeac4ef92673125dbcc16062c5b OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.
Failed to find vout address from OP_DUP OP_HASH160 3c819a9aa4a671e4dadd635abadeeb3124c7d6d3 OP_EQUALVERIFY OP_CHECKSIG
getdescriptorinfo error: There was an error. Check your console.

seems something is not right :(

@joeuhren
Copy link
Contributor Author

@2W-12 thanks for bringing this issue to my attention. A new commit has been posted that should resolve those issues. You will need to do a full resync, but it should sync without those getdescriptorinfo errors now.

@mboyd1
Copy link

mboyd1 commented Jun 27, 2022

this PR let me get past where I was stuck trying to sync the bitcoin testnet with iquidus, but now I am stuck at a much later block:

329672: d5eb9e18ee8dc9c8a29ed78533523aeb4fd4d763b1455c6cbcf18b5c6065f446
329672: dde925aec85386ee6118bb0832dd6c7082ce791027be3d219f0fc7e8a2e2e6cd
329672: dbce2efd02adc93a4ee0ee0dc5e53c83f6c2bea3c730e14f7a1365e0d33e887e
329672: e2ada40fe4ec88a4e90d0fd799ffe67430c67983ac61cad7e9e458ce4c5f9099
329672: 12df44cf355d7cedd6d2dbb5d777e9fd98afb277806469edb5e4a8b1c89fa9e3
329672: a6037a236f5adaf4ca34ca577c19579032265a3cf25b9848fdb25ea2a32502df
329672: a75358e84c0a3d043f269c7ac5b8e8f32b1b0bdf184be644ae2464cf616e88a3
/mnt/dockershare/iquidus/lib/explorer.js:484
      iteration:function(){
                        ^

RangeError: Maximum call stack size exceeded
    at Object.iteration (/mnt/dockershare/iquidus/lib/explorer.js:484:25)
    at /mnt/dockershare/iquidus/lib/explorer.js:582:20
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at /mnt/dockershare/iquidus/lib/explorer.js:589:14
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)
    at Object.syncLoop (/mnt/dockershare/iquidus/lib/explorer.js:492:10)
    at Object.is_unique (/mnt/dockershare/iquidus/lib/explorer.js:581:20)
    at processVoutAddresses (/mnt/dockershare/iquidus/lib/explorer.js:38:20)
    at /mnt/dockershare/iquidus/lib/explorer.js:652:11
    at Object.next (/mnt/dockershare/iquidus/lib/explorer.js:476:18)

@joeuhren
Copy link
Contributor Author

@mboyd1 the "RangeError: Maximum call stack size exceeded" error msg is addressed in the "Known Issues" section of the explorer README, although the first cmd is outdated and only works for older versions of nodejs.

Essentially you can run this cmd to find out what your current stack size is: node --v8-options | grep -B0 -A1 stack-size or node --v8-options | grep -B0 -A1 stack_size if using an older version of node (I'm no longer sure which version changed this, so try them both if you don't get a satisfactory result from the first)

Then, you will need to add an extra argument to your sync cmd with a higher value for the stack size than your default: node --stack-size=[SIZE] scripts/sync.js index update. Replace [SIZE] with the actual value you want to use. I think you will need to use a stack size of 5000 or maybe even 10000 to sync through a block that is completely full of transactions.

I never did finish syncing the full BTC blockchain myself because even with optimal hardware and the block_parallel_tasks feature enabled for 15 threads, it was still going to take a couple months to finish the sync and I have my doubts about whether mongo can handle such a large database because the explorer was extremely slow to operate after a while. One day I would like to see mongo replaced for something like MySQL to see if that helps with the operating speed for large blockchains like BTC.

@joeuhren joeuhren mentioned this pull request Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants