-
Notifications
You must be signed in to change notification settings - Fork 715
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
[RPC][MN] Serialize in ADDRV2 for TorV3 in RPC's #2873
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR works, unless there was a bip155 serialized address of length 13, which doesn't seem to be the case. Maybe instead of detecting the type "automatically", a flag can be added to the RPC?
tACK 6d3d535 because the above is a really small nitpick
I did think of trying to add a flag, but with the process in the RPCs and with these having the possibility of being done on separate daemons, the flag would not help us too much unless we are adding to the stream and can use that as an identifier for it. I did a fair bit of comparison between the encoded broadcasts between addrv1/2 and I haven't seen any signifiers for it, to make that change worth it/usable. Or with the flag, we would again need to have some insight into the hex string provided to us, but I can spend some more time to see if I overlooked anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must be more careful on this PR, I am sure that there are many places where we are still serializing with V1 only (and so this PR will probably only create new problems for torV3 masternodes).
Regardless of this possible problem, supporting two different serializations is useless. I think the easiest way to solve this issue is simply bumping the PROTOCOL_VERSION to post bip155 everywhere (of course at the next hard fork i.e v6.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 6d3d535 tested on Duddino's node throught MPW, works as intended.
Still for v6.0 I would really like to see v1 serialization being completely removed from the code
I agree and am looking into it. Have to do a fair bit of testing, for sanity of all types. But we have to continue to discuss a small cut-off so we can do the interim release possibly. Then the following can be bundled into 6.0 for the removal of v1 serialization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6d3d535
Did a couple local regtest TORv3 MNs
76fb659 Remove TorV2 checks for TorV3 addr verification (Liquid) Pull request description: This pull request creates TorV3 Address parsing functionality. TorV2 was depreciated nearly 2 years ago and should not have access to the Tor network anymore. Therefore we need to support TorV3 for users now in SPMT. Problems still arise, because this work relies on the work done in this PR as well: PIVX-Project/PIVX#2873 SPMT uses `decodemasternodebroadcast` and `relaymasternodebroadcast` which get updated in the above PR. We have removed TorV2 checks entirely because they should not be functional. Successful Broadcast Creation: ![image](https://github.com/PIVX-Project/PIVX-SPMT/assets/45834289/3ad52805-e59a-409d-bf1c-bea74584e8aa) Successfully Started: ![image](https://github.com/PIVX-Project/PIVX-SPMT/assets/45834289/ddb07b03-80d9-41b3-814e-5c97ee6d3981) Verification on daemon: ```pivx-cli -rpcconnect=127.0.0.2 getmasternodestatus { "txhash": "07b9b7beeb74c0bcef1d004c83b1ff741f776f2e4a6b6e8cf78ce7b8970d7b53", "outputidx": 0, "netaddr": "ovt6bxnzdj6afty22ccayzkuxnhhfuqjbn5gubmps4velhsxbmct6qyd.onion:51472", "addr": "DRndVQTocPwbPGRUzCUeFhXu36NerLsBHR", "status": 4, "message": "Masternode successfully started" } ``` Key things to TorV3 address parsing: Network ID as a PREFIX is 0x04 in determining its a TORV3 address through network messages Version is always '\x03' which is the 'd' at the end of the .onion address (last char before .onion) Length is always 62 characters and 35 bytes length. ACKs for top commit: Fuzzbawls: ACK 76fb659 Tree-SHA512: 6d8d544fb662cdd9b11cdaaf4b557d5e5a7a86d8e46de9719097c7b551937dfd7151ac91f558fd1d5e35dad41201ee92c6f9c95c6fa6154b8bf9633a7d082329
With some work being done in PIVX-Labs by @Duddino we noticed that the RPC's for decoding and creating master node broadcasts are in Pre-BIP 155 format or AddrV1 serialization.
In order for us to create support for Tor master nodes in MPW, we need these RPC's to be able to serialize a TorV3 address.
There's a few caveats for decoding that there may be other solutions provided by someone else, but for now since receiving the stream, we cannot tell if its PreBIP155 or not, so we unserialize by AddrV1 first and if it fails, we check and attempt AddrV2.