[Version-unspecific] Ensure that getOutput only returns an output with a vault address as either the sender or receiver
[Version-unspecific]
Intended to close #1682 (closed)
'UTXO getOutput can select outputs for which neither sender nor receiver is a vault address'.
However, at present local testing fails a lot of unit tests; I will look closer later, but in the meantime feedback is welcome.
(Intermediate updates have been saved for context, but hidden in a collapsible section to decrease visual clutter.)
https://gitlab.com/thorchain/thornode/-/jobs/5060880809 \
{"level":"info","module":"blockscanner","chain":"","block height":1,"time":"2023-09-11T16:47:48Z","message":"block scanner last fetch height"}
{"level":"info","module":"blockscanner","chain":"","block height":1,"time":"2023-09-11T16:47:49Z","message":"block scanner last fetch height"}
{"level":"error","chain":"DOGE","error":"fail to get asgards: fail to get asgards : fail to unmarshal pubkeys: unexpected end of JSON input","time":"2023-09-11T16:47:49Z","message":"fail to get asgard addresses"}
{"level":"error","chain":"DOGE","error":"fail to get asgards: fail to get asgards : fail to unmarshal pubkeys: unexpected end of JSON input","time":"2023-09-11T16:47:49Z","message":"fail to get asgard addresses"}
{"level":"error","chain":"DOGE","error":"fail to get asgards: fail to get asgards : fail to unmarshal pubkeys: unexpected end of JSON input","time":"2023-09-11T16:47:49Z","message":"fail to get asgard addresses"}
----------------------------------------------------------------------
FAIL: dogecoin_test.go:1025: DogecoinSuite.TestGetOutput
dogecoin_test.go:1055:
c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"fail to get output matching criteria"} ("fail to get output matching criteria")https://gitlab.com/thorchain/thornode/-/blob/v1.120.1/bifrost/thorclient/thorchain.go#L495-502
buf, err := b.getVaultPubkeys()
if err != nil {
return nil, fmt.Errorf("fail to get vault pubkeys ,err: %w", err)
}
var result stypes.QueryVaultsPubKeys
if err := json.Unmarshal(buf, &result); err != nil {
return nil, fmt.Errorf("fail to unmarshal pubkeys: %w", err)
}(As lint and regression tests all pass, perhaps this requires an ActiveVault SetVault at the beginning of the relevant unit tests.)
Update:
Hmm.
https://gitlab.com/thorchain/thornode/-/blob/31acd796/bifrost/pkg/chainclients/utxo/dogecoin_test.go#L1037-1056
In that TestGetOutput the sender is tb1qj08ys4ct2hzzc2hcz6h2hgrvlmsjynaw43s835 and the receiver is tb1qkq7weysjn6ljc2ywmjmwp8ttcckg8yyxjdz5k6.
In its SetUpTest:
https://gitlab.com/thorchain/thornode/-/blob/31acd796/bifrost/pkg/chainclients/utxo/dogecoin_test.go#L177-178 \
} else if strings.HasPrefix(req.RequestURI, thorclient.AsgardVault) {
httpTestHandler(c, rw, "../../../../test/fixtures/endpoints/vaults/asgard.json")https://gitlab.com/thorchain/thornode/-/tree/31acd796/test/fixtures/endpoints/vaults
has both asgard.json and pubKeys.json ,
so one might hope that there's already a pubkey corresponding to either the above sender or receiver in pubKeys.json,
needing just a few lines to include that pubKeys.json in SetUpTest.
https://gitlab.com/thorchain/thornode/-/blob/31acd796/test/fixtures/endpoints/vaults/pubKeys.json
Four Asgards, two Yggdrasils; no non-router addresses shown.
tthorpub1addwnpepqflvfv08t6qt95lmttd6wpf3ss8wx63e9vf6fvyuj2yy6nnyna576rfzjks
tthorpub1addwnpepq2flfr96skc5lkwdv0n5xjsnhmuju20x3zndgu42zd8dtkrud9m2vajhww6
tthorpub1addwnpepqwhnus6xs4208d4ynm05lv493amz3fexfjfx4vptntedd7k0ajlcunlfxxs
tthorpub1addwnpepq2mza4j4vplyjw295pkq8j2dan627lz6vufeu22pjx5vnnyjted5vwq3e3d
tthorpub1addwnpepq27s79a9xk8hjcpjuthmwnl2z4su43uynekcjuqcnmhpemfgfrh6sm4yd8e
tthorpub1addwnpepqtsgdw5dj7pj497vr2397pnfctf0d3lf7f2ssu39hts45567syh5xuz5jrs
https://gitlab.com/thorchain/thornode/-/blob/31acd796/test/fixtures/endpoints/vaults/asgard.json
There there are both pubkeys and addresses, but none of the pubkeys match the above six,
and there are no DOGE addresses.
getVaultPubkeys checks PubKeysEndpoint which is "/thorchain/vaults/pubkeys" ,
so I will try a reference to that pubKeys.json in case it resolves the matter.
Second update:
This approach is working, but just bringing TestGetOutput in line for example required replacing
{a tb1q address ostensibly representing a vault's DOGE address}
with an actual DOGE address derived from one of the pubKeys.json pubkeys.
As TestFetchTxs / TestGetMemPool / TestProcessReOrg are still failing, updating them for all chains could take a little while.
https://gitlab.com/thorchain/thornode/-/jobs/5061841399
Third update:
At present I have gotten through all the unit tests themselves, but face the same errors now originating from the later # network specific tests.
https://gitlab.com/thorchain/thornode/-/blob/v1.120.1/Makefile#L147-152
test:
@CGO_ENABLED=0 go test ${TEST_BUILD_FLAGS} ${TEST_DIR}
# network specific tests
@CGO_ENABLED=0 go test -tags stagenet ./common
@CGO_ENABLED=0 go test -tags testnet ./common ${BIFROST_UTXO_CLIENT_PKGS}
@CGO_ENABLED=0 go test -tags mainnet ./common ${BIFROST_UTXO_CLIENT_PKGS}While I feel it is reasonable to not extract (/observe) transactions which neither sender nor output receiver an observed vault,
if the same transaction is being used for unit tests testing Mocknet/Stagenet/Mainnet, then that transaction's address format can only satisfy one network structure.
(I have not forgotten that the original motivation was to,
when the sender is not an observed vault,
select as output a Vout which is to an observed vault
and no other output as the output.)
Perhaps one approach might be to comment out the txs.TxArray[0].To line of TestFetchTxs \ (for instance for Litecoin currently insisting that the To must be to address mv4rnyY3Su5gjcDNzbMLKBQkBicCtHUtFB`),
then to have identical transactions (with the other specified lines identical)
with different To addresses according to the different networks
(such that each network extracts its own corresponding transaction, and the different-network transactions all pass the same unit test's Asserts).
--Actually, the pipeline job itself passes? Perhaps distinct from make test, then, but for the sake of local make test passing I am still inclined to add a further commit.
Edit: Plausibly no commenting-out necessary; it was needed in any case for TestGetOutput to do network-dependent derivation of the expected address, so the same should be doable within TestFetchTxs (though adjacent near-identical transactions for the .json itself still needs to be tested).
623949bd
Fourth update:
Unit test edits have now gotten me as far as the Mainnet line. pubKeys.json as-is only has tthorpub pubkeys and is unusable for the prescribed Mainnet-specific tests.
Perhaps adding a Mainnet pubkey and checking for it will be sufficient, but I would greatly welcome the expression of others' thoughts regarding this.
https://gitlab.com/thorchain/thornode/-/commits/b18f0dca
FAIL: dogecoin_test.go:1034: DogecoinSuite.TestGetOutput
dogecoin_test.go:1036:
c.Assert(err, IsNil)
... value *fmt.wrapError = &fmt.wrapError{msg:"tthorpub1addwnpepqflvfv08t6qt95lmttd6wpf3ss8wx63e9vf6fvyuj2yy6nnyna576rfzjks is not bech32 encoded pub key,err : invalid Bech32 prefix; expected thorpub, got tthorpub", err:(*errors.errorString)(0xc00033c460)} ("tthorpub1addwnpepqflvfv08t6qt95lmttd6wpf3ss8wx63e9vf6fvyuj2yy6nnyna576rfzjks is not bech32 encoded pub key,err : invalid Bech32 prefix; expected thorpub, got tthorpub")Edit: Using pubKeys.json errors even when not set to Mainnet, if a Mainnet pubkey is present.
"fail to get asgards : fail to get asgards : fail to unmarshal pubkeys: thorpub1addwnpepqwprh5vd0rrk78kd98qjruuazwvapnxft7f86w7hlf768whxytpn5quf2gs is not bech32 encoded pub key
Should I then have an alternative pubKeys-Mainnet.json with the Mainnet pubkey in it I wonder...
Fifth update:
make test is now passing; branch updated.
Also, @ursa9r 's thoughts would be very welcome on the frequent calling of GetAsgardAddress (from isAsgardAddress)
and its addr, err := v.PubKey.GetAddress(chain)
in the context of
!2965 (diffs)
'Cache PubKey Addresses'.
((For instance, could that common-package NewChainPoolInfo cache code go directly into the common-package PubKey-receiving GetAddress function itself, instead of calling GetAddress?))
Edit: Pubkey address cache-generalisation commit now included; please tell me if it should be removed (and if so then why).
(Second edit: I note that I am pleased to read that getAsgardAddress() code already only allows call of GetAsgardAddress() approximately once a block.)
sync.Map note:
Learning about sync.Map has been intriguing in the pursuit of concurrent map reads; I note now-retracted commit
fe536c84
in case of interest/curiosity (and for convenient reference should I come back to this later).
|
While allowing concurrent reads at hypothetical times of high concurrency is appealing,
the prospect of numerous non-concurrent calls for different pubkeys doing extra reads or address-derivations
(depending on how the Lock is used)
is similarly unappealing, particularly noting that GetAsgardAddress calls GetAddress sequentially in a range for all observed pubkeys and not using goroutines.
|
Together with the (hopefully) negligible wait time for non-concurrent reads,
at this moment I lean towards the comparative simplicity of the original code's string-to-Address map and use of Lock for both reading and writing.
(That said, I welcome further food for thought, and will sleep on this.
2023-09-20 update:
Relaxation of counting rules requested by leena here;
I think this is appropriate and have included it it as a commit for this MR (rather than as a separate MR),
since the relaxed format rigidity relies on being able to select the right output by whether it's for a vault (when the sender isn't a vault).
up to 10 vOut with value
only 1 vOut needs to be Asgard, it can be anywhere
if one is an OP_RETURN, it can be anywhere
do not care where change goes
do not care where any out vOut goes
This is prompted by this replaced (and ignored by THORChain) transaction, for which a fee by the wallet used to replace the transaction created a third Vout:
https://www.blockchain.com/explorer/transactions/btc/aa74573cc525a02b48695e403d75f8580179e918ae53b82d057343a98e0b2710
https://www.blockchain.com/explorer/transactions/btc/3e9bf3b1e92732d75a58d42d43a07f9f45f3549daa18f57f021741a3a56a3414
In ignoreTx:
https://gitlab.com/thorchain/thornode/-/blob/v1.120.2/bifrost/pkg/chainclients/bitcoin/client.go#L914-917
countWithOutput := 0
for idx, vout := range tx.Vout {
if vout.Value > 0 {
countWithOutput++ // there are more than two output with value in it, not THORChain format
if countWithOutput > 2 {
return true
}As a comment, when there are two OP_RETURN memos (currently allowed, for memo-appending) they do need to be in order, as THORChain does not currently have a way of checking which should come first.
https://gitlab.com/thorchain/thornode/-/blob/v1.120.2/bifrost/pkg/chainclients/bitcoin/client.go#L1011-1039
// getMemo returns memo for a btc tx, using vout OP_RETURN
func (c *Client) getMemo(tx *btcjson.TxRawResult) (string, error) {
var opReturns string
for _, vOut := range tx.Vout {
if !strings.EqualFold(vOut.ScriptPubKey.Type, "nulldata") {
continue
}
buf, err := hex.DecodeString(vOut.ScriptPubKey.Hex)
if err != nil {
c.logger.Err(err).Msg("fail to hex decode scriptPubKey")
continue
}
asm, err := txscript.DisasmString(buf)
if err != nil {
c.logger.Err(err).Msg("fail to disasm script pubkey")
continue
}
opReturnFields := strings.Fields(asm)
if len(opReturnFields) == 2 {
decoded, err := hex.DecodeString(opReturnFields[1])
if err != nil {
c.logger.Err(err).Msgf("fail to decode OP_RETURN string: %s", opReturnFields[1])
continue
}
opReturns += string(decoded)
}
}
return opReturns, nil
}