cardano-foundation / cardano-wallet

HTTP server & command-line for managing UTxOs and HD wallets in Cardano.
Apache License 2.0
759 stars 211 forks source link

metadata hash mismatch / no re-check #1855

Closed gufmar closed 4 years ago

gufmar commented 4 years ago

Context

cardano-wallet does complain about metadata hash mismatch, and then don't refresh/recheck again

Information -
Version v2020.7.6 (git revision: 298f91d677658e5c69066daed35fec719164ab51)
Platform Windows
Installation as part of Daedalus 1.2.0

Steps to Reproduce

Earlier this morning I wanted to check if pool CLIO1 show up in cardano-wallet/Daedalus and might register if first registration was done before cardano-cli 1.14.2

[cardano-wallet.pools-engine:Info:48] [2020-07-05 09:18:02.87 UTC] Fetching metadata with hash 2a860a13 from https://clio.one/metadata/clio1.json
[cardano-wallet.pools-engine:Warning:48] [2020-07-05 09:18:03.06 UTC] Failed to fetch metadata from https://clio.one/metadata/clio1.json: Metadata hash mismatch. Saw: 46e4da61de7f085581d3cccdd63a3a28ebc6e7abe567d771d8601bb9afa4cab5, but expected: 2a860a13ae83c460570c8d7f56562d3eedd50fe48a717d350412c84ff2b61c61

based on this result I decided to re-register the pool by using

cardano-cli 1.14.2 - linux-x86_64 - ghc-8.6
git rev a1c000025cb1f6d66f0e4b058e82edd89aee8a10

As the description in metadata for the first registration contained a ' I decided to remove it for the re-registration and saved the new metadata file at the same URL https://clio.one/metadata/clio1.json

This resulted in a new fetch and complaint about two different non matching hashes from cardano-wallet

[cardano-wallet.pools-engine:Info:26] [2020-07-05 09:25:52.82 UTC] Fetching metadata with hash 2ecfcb90 from https://clio.one/metadata/clio1.json
[cardano-wallet.pools-engine:Warning:26] [2020-07-05 09:25:53.00 UTC] Failed to fetch metadata from https://clio.one/metadata/clio1.json: Metadata hash mismatch. Saw: 74fe63b5ebbde8930244ff142771c203a63c252141cfc24deac79723b0988ab8, but expected: 2ecfcb903544e65a37ab6792b82f105d15a587e6a02877872ff2d2fe195bf02c
[cardano-wallet.pools-engine:Info:26] [2020-07-05 09:25:53.02 UTC] Taking a little break from fetching metadata, back to it in about 20.0s

since then I restarted cardano-node multiple times but there was no further attempt logged to fetch clio1 metadata file.

I also verified the metadata files on webserver as well as delivered by HTTPS request generate the expected hash. (2a86... for first and 2ecf... for second registration) So I can't explain where and how cardano-wallet get the metadata file and compute a different hash than expected. I also don't know how to enforce a new check

gufmar commented 4 years ago

even after 8 more hours and some restarts the metadata file wasn't loaded and verified again. There were no more traces in the log file see attached pub.zip) I removed the application and renamed the whole Daedalus...v2 folder in appdata/roaming While syncing up from genesis it found the first (obsolete pre 1.14.2) certificate and logged a hash mismatch for the new metadata file. Then after catching up to today's epochs' the pool appeared in the list of pools. pub.zip

KtorZ commented 4 years ago

@gufmar Was your registered metadata hash the same in this two registration certificates? (not talking about the actual hash, but the one written in the certificate?)

That metadata hash is used as a primary key when it comes to caching fetch attempts on metadata, behind an exponential backoff; so if the registered metadata hash was the same, that would explain it (your first attempts "burnt" all your cheap credits, and the next retry was scheduled in quite a while...).

gufmar commented 4 years ago

This was a two step process from my side

I first registered on chain with a pre 1.14.2 version. Also placed the metadata file to the registered URL.

Daedalus 1.2 didn't show this first registration and file at that point

Log: 09:18:02 saw 46e4... expected 2a86...

As this first versions Description field contained an ' (...what's... ) I decided to remove such a potential breaker character and do a second registration with 1.14.2 I also updated the metadata file at the same URL.

No show in Daedalus and wallet log said

Log: 09:25:02 saw 74fe... expected 2ecf...

I also verified no intermediate proxy or anything else causes little changes like removed line breaks and spacers (eg cloudflare performance tools)

From that moment on, even after multiple restarts and +8h wait time there was no more sign of reload, also no msg to skip because...

Only after installing from scratch with empty folder it worked as expected.

Imo the cli checksum and wallet verification should be done on the content fields only. Introduce a metadata version field for future enhancements, and then define version 1 has 4 fields in this order: name; ticker;description;homepage Max length is xx now do a checksum on exactly this CSV like string both for chain reg and for metadata verification. The metadata file can and should still remain JSON.

KtorZ commented 4 years ago

As this first versions Description field contained an ' (...what's... ) I decided to remove such a potential breaker character and do a second registration with 1.14.2

cardano-wallet & Daedalus supports UTF-8 just fine :)

From that moment on, even after multiple restarts and +8h wait time there was no more sign of reload, also no msg to skip because...

One thing you have to keep in mind is that, you're not the only pool. The wallet is reaching out to over more than 300 servers at the moment, so it can't allow to wait for all of them to kindly reply, especially when most are either unimplemented or are returning bad data. This is actually a quite big surface of attack and the wallet is being quite defensive about it.

now do a checksum on exactly this CSV like string both for chain reg and for metadata verification.

We've kept the metadata registration process quite simple in the end, which turned out to be already quite an obstacle for pool operators (out of 300, only 40 had gotten the metadata hash right last week). We discussed using canonical JSON exactly to avoid issues with key swapping / deserialization discrepancies between JSON parsers and decided not to in the end as many people have actually never heard of canonicalization in the first place.

Introduce a metadata version field for future enhancements

Now, what would be the added value of such a versioning? One would still have to re-upload a metadata file with a different hash regardless. Should you find the time, I'd highly suggest to write down your idea as a Cardano Improvement Proposal since this is something that reaches beyond cardano-wallet. So far, the metadata format is implicitly agreed upon, and documented somewhere in the 100+ pages of Shelley design specs. Yet, there's no on-chain limitation with regards to the structure of that file, so we are free to change this and a well-written proposal around which there can be a discussion would totally make sense.

gufmar commented 4 years ago

One thing you have to keep in mind is that, you're not the only pool.

and

I'd highly suggest to write down your idea

please forgive when I tell you now that based on my experiences I will not do this anymore.

The Design specs have a paragraph about this topic.

image

and so I thought it might is worth to start such an initiative, by also considering the performance and attack vector issue for wallets. I prepared it as a proposal for Metadata proxies for collaboration and development, by also considering some ideas from Emurgo Yoroi team about ways to fetch and show (curated) metadata.

Now guess what's the best way to slash such an idea?

For now and with given dates I believe it's best to go with what was chosen with on-chain URL and everyone host his little json file.

I might also recommend to all pools to use a versioned URL to metadata for every re-registration, when metadata content changes. This will prevent cardano-wallet to fetch a new metadata file, while bootstrapping from genesis and find all historic pool registrations with outdated hashes.

KtorZ commented 4 years ago

IOHK started some work in the direction of a proxy. It's a little project called "SMASH" (https://github.com/input-output-hk/smash/). It's basically a server that fetches metadata from peers, cache them and have a little API to return them on demand based on the hash. The setup is a little heavy as it requires a node and a cardano-db-sync running, but that something which could be used to power proxies hosted by community members.

Now guess what's the best way to slash such an idea?

This seems quite opposite to "please forgive when I tell you now that based on my experiences I will not do this anymore.". My suggestion still is to write a CIP explaining your idea.

gufmar commented 4 years ago

Sorry, I wanted to write "how to _SMASH_such an idea" as a pun. :-)

I started working on the metadata proxy back in January 2020 after I found it mentioned in the specs. And it probably was also my first realization from the ITN: we should do better than the github pullrequest based metadata registry. CIPs first appeared only 4 months later.

When I started nobody knew anything about the plans to provide all pools metadata in a performant way to all kind of wallets, explorers, mobile apps, tickers, bots, ... and in the best way also by different "metadata providers" in a curated form. Eg marking legal, fraud, offending, misleading, double-signing players. Or on the positive side certain quality badges. This idea came from Emurgo side (see https://github.com/Emurgo/EmIPs/blob/11bf63e3f6501cf9ede3ec2c503985dbd52e2b0f/specs/emio-006.md) Adding such criteria would allow applications to provide more search filters to the end-users. End users could freely decide if they want a fully uncensored view or trust some 3rd party curators, protecting them from possibly unwanted pools.

Hence the idea of having version numbers and standardized metadata structure. The fundamental pool data from the chain (ticker, cost, margin) plus poolside hosted metadata, could be enriched by third parties, on his way to consuming applications who have their own ideas and demands. In a decentralized environment introducing and upgrading to a new version always require some dual-stack or backwards-compatibility planning.

I presented it in the community, discussed possible (missing) incentives, efforts and risks to run such a metadata proxy mid/long term. I also presented it at the Meetup in London in February.

Then sometime in May, I heard about Project Smash for the first time. Nobody asked me about my experiences, ideas and the feedback I've received so far (probably nobody was aware or watched community channels ) So I decided to leave this to the professionals, the fully paid developers of Cardano, because now, 6 months later there is not enough time to develop such ideas and provide it to the chain and the app developers as a ready infrastructure solution. It would require at least one such metadata-proxy-curator to start. But in reality, we would need more of them, to prevent SPOFs and gateways.
This brings up the question if it was a good idea to start implementing SMASH in Haskell and Nix, because it will be anything than trivial for metadata curators to contribute and extend it for their own diversification ideas. Instead of implementation, in my opinion, a workgroup with clear information about the planned data API (now we know cardano-db-sync) and community efforts would have been a better approach.

So, I fully agree: an improvement proposal makes sense. Question is if it's considered then, and what we do now first, short term to have something in the MVP mainnet release.

KtorZ commented 4 years ago

we should do better than the github pull request based metadata registry

Hell yeah. You can thank me for that (or blame me :upside_down_face:). We were quite in a hurry for the ITN launch and there had been unforeseen complications regarding pools metadata. So we needed a quick'n'dirty solution that would work. I wonder how github didn't block this repository, but it kinda did the job the ITN.

This was however never intended to be a long-term solution. It is wrong in many ways.

Then sometime in May, I heard about Project Smash for the first time. Nobody asked me about my experiences, ideas and the feedback I've received so far (probably nobody was aware or watched community channels )

Indeed. We (a.k.a the engineering team) do not watch community channels as much as we would like. I, like many others, sometime try to be present on Reddit, Telegram, The Forum or Slack but it's kinda hard for us. This is also why we pushed so much for getting this CIP process out. The ITN has shown that there were members in the community that wanted to do stuff. We also needed this to organize between companies with "competing" softwares (e.g. Daedalus & Yoroi).

This brings up the question if it was a good idea to start implementing SMASH in Haskell

This is a legitimate question although there's not much choice here yet. In order to implement such a service, you need to be able to connect to a node and process blocks from that node. Or, you need to rely on a service that does so (like cardano-db-sync or cardano-graphql). Smash is written in Haskell because that's what we know best around here :upside_down_face: and because it makes a lot of sense to build a webserver in Haskell (warp is actually a real beast and a great piece of engineering). Now, it's perhaps not the best choice with regards to community engagement and open-source contributions. SMASH is more of a service that can be consumed and ran as-is.

extend it for their own diversification ideas. Instead of implementation, in my opinion, a workgroup with clear information about the planned data API

Note that SMASH in itself is pretty small as it heavily rely on the data present in the postgresql database dumped by cardano-db-sync. One could rebuild such service using Python, Node.js or alike. What's important is to agree on interfaces and the format of data flowing through the service. This is why I think a CIP that specifies both the expected HTTP interface, and the structure of the metadata file would offer a very nice foundation for discussions. This is something I'll eventually end up writing and submitting, but it'll basically specify what SMASH does, on top of which, I am more than happy to see another proposal or an extension to discuss versioning and possible new fields and semantic.

KtorZ commented 4 years ago

Closing this as the behavior has changed. Both the URL and the hash are used as primary key and publishing a new certificate with either a different hash or a different URL will therefore go over the fetching phases again.