-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
@joeuhren Thank you! Will test this on my branch soon or commit if others have tested/approved |
Works great on a chain I used to have to use some workaround code from other Pull Requests to sync - nice work! |
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 |
One issue I've seen is that 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. |
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 @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. |
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 |
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! |
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. |
@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. |
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. |
After this commit I can see a lot of errors:
seems something is not right :( |
@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. |
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:
|
@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: Then, you will need to add an extra argument to your sync cmd with a higher value for the stack size than your default: I never did finish syncing the full BTC blockchain myself because even with optimal hardware and the |
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