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

bech32 disintegration issue #23

Open
patricklodder opened this issue Apr 5, 2021 · 14 comments
Open

bech32 disintegration issue #23

patricklodder opened this issue Apr 5, 2021 · 14 comments

Comments

@patricklodder
Copy link
Contributor

Hi. I replaced bitcoin addresses with (what I believe) should be Dogecoin's bech32-encoded address in construction_service_test.go (got these "tdge" addresses from information in error stack trace. Anyways, the error message I get is:

Error:      	Expected nil, but got: &types.Error{Code:10, Message:"Unable to decode address", Description:(*string)(nil), Retriable:false, Details:map[string]interface {}{"context":"decoded address is of unknown format unable to decode address tdge1q3r8xjf0c2yazxnq9ey3wayelygfjxpfqmg6g08"}}

But after test process completes I get the following message:

  	Test:       	TestConstructionService
 construction_service_test.go:412: PASS:	SuggestedFeeRate(*context.emptyCtx,int64)
 construction_service_test.go:412: PASS:	SuggestedFeeRate(*context.emptyCtx,int64)
 construction_service_test.go:412: PASS:	SendRawTransaction(*context.emptyCtx,string)
 construction_service_test.go:413: PASS:	GetScriptPubKeys(*context.emptyCtx,[]*types.Coin)
 construction_service_test.go:413: PASS:	GetScriptPubKeys(*context.emptyCtx,[]*types.Coin)
FAIL
FAIL	github.com/rosetta-dogecoin/rosetta-dogecoin/services	1.083s
ok  	github.com/rosetta-dogecoin/rosetta-dogecoin/indexer	(cached)
ok  	github.com/rosetta-dogecoin/rosetta-dogecoin/bitcoin	(cached)
ok  	github.com/rosetta-dogecoin/rosetta-dogecoin/dogecoin	(cached)
FAIL
make: *** [Makefile:72: test] Error 1

I am not sure why "Unable to decode address" error comes up when the error messages themselves contain addresses with "tdge" prefix in them as suggested option? Here's an example of one the messages included when running 'make test':

	  AccountIdentifier: (*types.AccountIdentifier)({
        	            	-  Address: (string) (len=34) "DSpgzjPyfQB6ZzeSbMWpaZiTTxGf2oBCs4",
        	            	+  Address: (string) (len=44) "tdge1qj8qlgw4qtnxxgxl5nzs5n56fxkqg2e5rnqc6kn",

I replaced "DSpgzjPyfQB6ZzeSbMWpaZiTTxGf2oBCs4" with "tdge1qj8qlgw4qtnxxgxl5nzs5n56fxkqg2e5rnqc6kn" but still getting "Unable to decode address" when using one with "tdge" prefix. Any ideas why?

Thanks,
Victor.

Originally posted by @victorsk2019 in #1 (reply in thread)

@godcloutier
Copy link

hey there im gonna check it out trying to get up to speed ;)

@godcloutier
Copy link

I saw in the btcd params that "tb" is required that may also be one of the reason why

@patricklodder patricklodder added this to the [2] Compliant API milestone Apr 14, 2021
@incognitojam
Copy link
Contributor

Looks like we never registered our network params with btcd. We can do that in dogecoin/params.go by calling chaincfg.Register, which allows btcd to recognise the format tdge as a Bech32 prefix. The test fails with a different error now:

construction_service_test.go:302: 
    	Error Trace:	construction_service_test.go:302
    	Error:      	Expected nil, but got: &types.Error{Code:10, Message:"Unable to decode address", Description:(*string)(nil), Retriable:false, Details:map[string]interface {}{"context":"checksum failed. Expected mg6g08, got jvj5v7. unable to decode address tdge1q3r8xjf0c2yazxnq9ey3wayelygfjxpfqjvj5v7"}}
    	Test:       	TestConstructionService

I also noticed a typo in the params so will submit a PR to fix that.

@incognitojam
Copy link
Contributor

Replacing "tb" with "tdge" and updating the Bech32 checksums for each of the addresses solves most of the errors bar two, after applying the PR #32.

construction_service_test.go:313: 
    	Error Trace:	construction_service_test.go:313
    	Error:      	Not equal: 
    	            	expected: &types.ConstructionPayloadsResponse{UnsignedTransaction:"7b227472616e73616374696f6e223a2230313030303030303031376639636635306230326464353235386638306364356333343337333032653032376464313333363137326132306364633830333035633561353537343162313031303030303030303066666666666666663032646239313065303030303030303030303136303031343838636536393235663835313361323334633035633932326565393333663232313332333035323037316165303030303030303030303030313630303134393430373236353935633431666361306234383130633632393931616439643238396565623832383030303030303030222c227363726970745075624b657973223a5b7b2261736d223a22302063303035623030616430373564333062383961376236356237646164383839396261366139633535222c22686578223a223030313463303035623030616430373564333062383961376236356237646164383839396261366139633535222c2272657153696773223a312c2274797065223a227769746e6573735f76305f6b657968617368222c22616464726573736573223a5b227462317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34666b6c68766d225d7d5d2c22696e7075745f616d6f756e7473223a5b222d31303030303030225d2c22696e7075745f616464726573736573223a5b227462317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34666b6c68766d225d7d", Payloads:[]*types.SigningPayload{(*types.SigningPayload)(0xc0001e4390)}}
    	            	actual  : &types.ConstructionPayloadsResponse{UnsignedTransaction:"7b227472616e73616374696f6e223a2230313030303030303031376639636635306230326464353235386638306364356333343337333032653032376464313333363137326132306364633830333035633561353537343162313031303030303030303066666666666666663032646239313065303030303030303030303136303031343838636536393235663835313361323334633035633932326565393333663232313332333035323037316165303030303030303030303030313630303134393430373236353935633431666361306234383130633632393931616439643238396565623832383030303030303030222c227363726970745075624b657973223a5b7b2261736d223a22302063303035623030616430373564333062383961376236356237646164383839396261366139633535222c22686578223a223030313463303035623030616430373564333062383961376236356237646164383839396261366139633535222c2272657153696773223a312c2274797065223a227769746e6573735f76305f6b657968617368222c22616464726573736573223a5b2274646765317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34716a6874307a225d7d5d2c22696e7075745f616d6f756e7473223a5b222d31303030303030225d2c22696e7075745f616464726573736573223a5b2274646765317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34716a6874307a225d7d", Payloads:[]*types.SigningPayload{(*types.SigningPayload)(0xc0001e4120)}}
    	            	
    	            	Diff:
    	            	--- Expected
    	            	+++ Actual
    	            	@@ -1,3 +1,3 @@
    	            	 (*types.ConstructionPayloadsResponse)({
    	            	- UnsignedTransaction: (string) (len=1122) "7b227472616e73616374696f6e223a2230313030303030303031376639636635306230326464353235386638306364356333343337333032653032376464313333363137326132306364633830333035633561353537343162313031303030303030303066666666666666663032646239313065303030303030303030303136303031343838636536393235663835313361323334633035633932326565393333663232313332333035323037316165303030303030303030303030313630303134393430373236353935633431666361306234383130633632393931616439643238396565623832383030303030303030222c227363726970745075624b657973223a5b7b2261736d223a22302063303035623030616430373564333062383961376236356237646164383839396261366139633535222c22686578223a223030313463303035623030616430373564333062383961376236356237646164383839396261366139633535222c2272657153696773223a312c2274797065223a227769746e6573735f76305f6b657968617368222c22616464726573736573223a5b227462317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34666b6c68766d225d7d5d2c22696e7075745f616d6f756e7473223a5b222d31303030303030225d2c22696e7075745f616464726573736573223a5b227462317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34666b6c68766d225d7d",
    	            	+ UnsignedTransaction: (string) (len=1130) "7b227472616e73616374696f6e223a2230313030303030303031376639636635306230326464353235386638306364356333343337333032653032376464313333363137326132306364633830333035633561353537343162313031303030303030303066666666666666663032646239313065303030303030303030303136303031343838636536393235663835313361323334633035633932326565393333663232313332333035323037316165303030303030303030303030313630303134393430373236353935633431666361306234383130633632393931616439643238396565623832383030303030303030222c227363726970745075624b657973223a5b7b2261736d223a22302063303035623030616430373564333062383961376236356237646164383839396261366139633535222c22686578223a223030313463303035623030616430373564333062383961376236356237646164383839396261366139633535222c2272657153696773223a312c2274797065223a227769746e6573735f76305f6b657968617368222c22616464726573736573223a5b2274646765317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34716a6874307a225d7d5d2c22696e7075745f616d6f756e7473223a5b222d31303030303030225d2c22696e7075745f616464726573736573223a5b2274646765317163717a6d717a6b7377686673687a64386b6564686d7476676e78617834387a34716a6874307a225d7d",
    	            	  Payloads: ([]*types.SigningPayload) (len=1) {
    	Test:       	TestConstructionService
construction_service_test.go:325: 
    	Error Trace:	construction_service_test.go:325
    	Error:      	Not equal: 
    	            	expected: &types.ConstructionParseResponse{Operations:[]*types.Operation{(*types.Operation)(0xc0000accc0), (*types.Operation)(0xc0000acd20), (*types.Operation)(0xc0000acd80)}, AccountIdentifierSigners:[]*types.AccountIdentifier{}, Metadata:map[string]interface {}(nil)}
    	            	actual  : &types.ConstructionParseResponse{Operations:[]*types.Operation{(*types.Operation)(0xc0000acea0), (*types.Operation)(0xc0000acf00), (*types.Operation)(0xc0000acf60)}, AccountIdentifierSigners:[]*types.AccountIdentifier{}, Metadata:map[string]interface {}(nil)}
    	            	
    	            	Diff:
    	            	--- Expected
    	            	+++ Actual
    	            	@@ -11,3 +11,3 @@
    	            	    Account: (*types.AccountIdentifier)({
    	            	-    Address: (string) (len=44) "tdge1qcqzmqzkswhfshzd8kedhmtvgnxax48z4qjht0z",
    	            	+    Address: (string) (len=42) "tb1qcqzmqzkswhfshzd8kedhmtvgnxax48z4fklhvm",
    	            	     SubAccount: (*types.SubAccountIdentifier)(<nil>),
    	Test:       	TestConstructionService

@victorsk2019
Copy link

victorsk2019 commented Apr 15, 2021

Looks like we never registered our network params with btcd. We can do that in dogecoin/params.go by calling chaincfg.Register, which allows btcd to recognise the format tdge as a Bech32 prefix.

As I understand, Register() in original btcd has '1' hard-coded in the address:

     // A valid Bech32 encoded segwit address always has as prefix the
     // human-readable part for the given net followed by '1'.
     bech32SegwitPrefixes[params.Bech32HRPSegwit+"1"] = struct{}{}

I thought to override Register() in params.go and add required function calls locally. Here's my attempt to modify params.go:

var (   
        registeredNets       = make(map[wire.BitcoinNet]struct{})
        pubKeyHashAddrIDs    = make(map[byte]struct{})
        scriptHashAddrIDs    = make(map[byte]struct{})
        bech32SegwitPrefixes = make(map[string]struct{})
        hdPrivToPubKeyIDs    = make(map[[4]byte][]byte)
)

func Register(params *chaincfg.Params) error {
        
       //causes error, disabled temporarily
       /*if _, ok := registeredNets[params.Net]; ok {
                return ErrDuplicateNet
        }*/
        registeredNets[params.Net] = struct{}{}
        pubKeyHashAddrIDs[params.PubKeyHashAddrID] = struct{}{}
        scriptHashAddrIDs[params.ScriptHashAddrID] = struct{}{}

         //temporarily disabled
        /*err := RegisterHDKeyID(params.HDPublicKeyID[:], params.HDPrivateKeyID[:])
        if err != nil {
                return err
        }*/

        // A valid Bech32 encoded segwit address always has as prefix the
        // human-readable part for the given net followed by '1'.

        // Removed '1' 
        bech32SegwitPrefixes[params.Bech32HRPSegwit] = struct{}{}
        return nil
}

// mustRegister performs the same function as Register except it panics if there
// is an error.  This should only be called from package init functions.
func mustRegister(params *chaincfg.Params) {
        if err := Register(params); err != nil {
                panic("failed to register network: " + err.Error())
        }
}

func init() {
        // Register Testnet network
        mustRegister(&TestNet3Params)
}

But I'm still getting the same address error. Not sure if I'm on the right track with this approach?

@incognitojam
Copy link
Contributor

@victorsk2019 Sorry I missed your message about the 1 in the prefix! I don't think copying the maps into our params.go is workable because the btcd methods all reference the original variables.

@incognitojam
Copy link
Contributor

Are you sure the 1 prefix is bad? It looks like other coins have the prefix and address separated by the 1. Maybe this is just Bech32 standard. [1] [2]

@victorsk2019
Copy link

Are you sure the 1 prefix is bad? It looks like other coins have the prefix and address separated by the 1. Maybe this is just Bech32 standard. [1] [2]

Thank you for letting me know. I assumed it was bitcoin-specific and it also says in the comments it's part of Bech32 standard :/

@victorsk2019
Copy link

victorsk2019 commented Apr 16, 2021

Sorry, may I ask how you obtained values for MainNet and TestNet3 for PR #32?

// Constants used to indicate the message dogecoin network.
const (
        // MainNet represents the main dogecoin network.
        MainNet wire.BitcoinNet = 0xc0c0c0c0

        // TestNet3 represents the test network (version 3).
        TestNet3 wire.BitcoinNet = 0xdcb7c1fc
)

I was looking for these value occurrence in dogecoin-core (and btcd project) thinking maybe it was obtained from there but got nothing using this command:

grep -r "0xdcb7c1fc" .

Just curious how these values were derived? Thanks.

@incognitojam
Copy link
Contributor

It's here in chainparams.cpp! Not as one continuous number but as an array so not easy to find 😄 . Testnet one is here as well.

@victorsk2019
Copy link

As per discussion in issue #1, it doesn't appear that doge testnet currently supports bech32/segwit address format? Maybe modifying construction_service_test.go to work with base58 address instead could be a better approach? Coinbase addresses seem to be in base58 so it doesn't seem to be that important for us to have bech32 support functional in order to deploy Doge on Coinbase. I am not sure, but I thought passing test cases would imply readiness to deploy to production.

@solo-fish
Copy link
Contributor

It seems this is a quite common issue for rosetta implementations, I think this is the most important take-away:

Rosetta doesn't really have a notion of different address types and it is creating some confusion for folks trying to integrate with rosetta-bitcoin using non Segwit-Bech32 address formats

-coinbase/rosetta-specifications/#71

@victorsk2019
Copy link

victorsk2019 commented May 6, 2021

Quick note: Just checked updates on main branch and it seems construction_service_test.go has finally been fixed (thank you xanimo for this contribution). 🎉🚀

make test go test ./services/... ./indexer/... ./bitcoin/... ./dogecoin/... ok github.com/rosetta-dogecoin/rosetta-dogecoin/services (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/indexer (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/bitcoin (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/dogecoin (cached)

Since the transactions testing functionality now seems to be working, are we good for deployment to Coinbase or is there anything else that needs to be done?

@xanimo
Copy link
Contributor

xanimo commented May 6, 2021

Quick note: Just checked updates on main branch and it seems construction_service_test.go has finally been fixed (thank you xanimo for this contribution). 🎉🚀

make test go test ./services/... ./indexer/... ./bitcoin/... ./dogecoin/... ok github.com/rosetta-dogecoin/rosetta-dogecoin/services (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/indexer (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/bitcoin (cached) ok github.com/rosetta-dogecoin/rosetta-dogecoin/dogecoin (cached)

Since the transactions testing functionality now seems to be working, are we good for deployment to Coinbase or is there anything else that needs to be done?

A few of us are running our initial and 2nd testnet syncs to see what the issue @patricklodder encountered in #48

Outside of that not sure :/

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

No branches or pull requests

6 participants