cardano-ledger-core's decodeAddr / deserialiseAddr fail to deserialize mainnet pointer address

[KtorZ]

package	version
cardano-ledger-core	1.8.0.0

Important

That there's no changes on the impacted module in 1.9.0.0, so while I haven't tried with 1.9.0.0 I hardly believe it fixes the problem.

It seems that since the internals of the Cardano.Ledger.Address module was reworked to utilize state transformers, something broke regarding the parsing of pointer addresses.

More specifically, trying to decode (the bech32-decoded representation of): addr1gy5p8wv6sr8mgqjrwj7s75pft9y94ftwqey9vnlcqhew2xaumxqe2cdam3npgv60hqa (or 412813b99a80cfb4024374bd0f502959485aa56e0648564ff805f2e51bbcd9819561bddc6614 in base16) using decodeAddr yields an error.

https://github.com/input-output-hk/cardano-ledger/blob/45c67892540d52aa6af9972c58bb20a125ceea09/libs/cardano-ledger-core/src/Cardano/Ledger/Address.hs#L142-L143

https://github.com/input-output-hk/cardano-ledger/blob/45c67892540d52aa6af9972c58bb20a125ceea09/libs/cardano-ledger-core/src/Cardano/Ledger/Address.hs#L565-L571

However, that address is an actual mainnet address found in one of the blocks of the Mary era. It can also be properly decoded using old code as well as alternative implementations like the one from cardano-address for de-serializing pointers.

From a quick read through the ledger's code, it seems that the problem could come from how the new decoders assume a certain length for the pointers in decodeVariableLengthWord16 and decodeVariableLengthWord32. This leads to always decoding the same number of bytes whereas the encoding is meant to work with variable-length naturals and thus, may end up shorter than expected here.

https://github.com/input-output-hk/cardano-ledger/blob/45c67892540d52aa6af9972c58bb20a125ceea09/libs/cardano-ledger-core/src/Cardano/Ledger/Address.hs#L815

[lehins]

This change was intentional. Deserializer for Ptrs was buggy and it had to be fixed.

There were a total of 10 addresses on mainnet that contained Ptrs, only a few of them where legitimately valid pointers. @KtorZ I suspected some of those addresses were yours 😉

This particular address contains this bogus pointer (note the transaction index is invalid and SlotNo is too big):

Ptr (SlotNo 16292793057) (TxIx 1011302) (CertIx 20)

Starting with Conway, transaction index (TxIx) and certificate index (CertIx) will be backed by Word16 and SlotNo by Word32 (for the Ptr purpose), so this kind of bogus pointers will no longer even be possible, therefore in Babbage we have disabled new pointers like this from being created at all. Going into Conway this Ptr, along with other invalid pointers, will be translated to all zeros: Ptr (SlotNo 0) (TxIx 0) (CertIx 0)

Moreover, starting with Conway, Ptr address will no longer work at all for their intended purpose of delegation. We'll still allow them to be part of the address for backwards compatibility with plutus scripts, except they will have no meaning in ledger.

This feature of pointer addresses was a total disaster that caused more problems than it solved, so I am really glad that it is going away.

[lehins]

I am gonna close this as won't fix. Let me know if you still feel like this is a mistake and you have some strong argument for bringing this buggy behavior back. Considering that pointers are being deprecated I can't see any argument in favor of allowing nonsense like this on-chain.

[KtorZ]

I am fully onboard with the idea of how pointer addresses were a bad (or at least prematurely pushed) idea, and yes, one of these addresses is mine (not this one though, mine is properly constructed ☺️).

That's not the point of debate though. Here, the API of the ledger is no longer able to parse data that exists on chain (and will forever exist). Which means that it is no longer possible to do a proper chain sync across the old eras. Pointers being dropped in Conway is no justification to drop support for them in previous era! If I may, this was the point of having a hard fork combinator to begin with. How do we expect client applications to synchronize through the old chain? Even invalid pointers are data present in blocks and should be deserialisable.

[lehins]

Which means that it is no longer possible to do a proper chain sync across the old eras. Pointers being dropped in Conway is no justification to drop support for them in previous era! If I may, this was the point of having a hard fork combinator to begin with. How do we expect client applications to synchronize through the old chain? Even invalid pointers are data present in blocks and should be deserialisable.

No worries. It all works. This change has been in ledger since last year. Syncing works and ledger is still able to decode that address. Decoder only disables addresses like these to be used in new TxOuts starting from Babbage.

[KtorZ]

Then why does the ledger fail to parse this particular address? I would expect decodeAddr to succeed given that it is a valid address (by the very fact that it is in a block which is years old).

[KtorZ]

Given that addresses are era independent here (only depends on crypto), how is the ledger then selecting the correct decoder for old eras? And how are client applications and consumers of the ledger library supposed to deal with that?

[lehins]

Then why does the ledger fail to parse this particular address? I would expect decodeAddr to succeed given that it is a valid address (by the very fact that it is in a block which is years old).

decodeAddr is not used for transaction deserialization. This is just a function that was exported for convenience.

fromCborAddr is the decoder that is used for deserialization in the ledger. Decoders in ledger have versioning capability and can take different branch depending on the current protocol version, which means we can use lenient decoding for older eras and stricter for latest eras.

If you would like a to have access to a more lenient version of decodeAddr that can handle such old buggy addresses we can certainly export it from cardano-ledger-core. Honestly, I wasn't even sure if anyone uses that function, considering cardano-addresses implements it's own deserialization.

Given that addresses are era independent here (only depends on crypto), how is the ledger then selecting the correct decoder for old eras?

Addresses only make sense in ledger as part of transaction, which is era dependent. Binary representation of every single thing that goes on chain is era dependent.

And how are client applications and consumers of the ledger library supposed to deal with that?

I have no idea how client applications and consumers use ledger code, so we cannot accommodate everyone. That being said, there are two ways we can solve this particular problem:

• We can either remove decodeAddr function completely and that would solve the problem, since we don't need it for ledger functionality.
• Or we can provide extra functions that provide lenient decoding, for applications that want to deal with those buggy addresses.

I am leaning to the former solution, since that would be less work for us. On the other hand, I would like for ledger codebase to be more user friendly, so I would not be opposed to exporting those helper functions from cardano-ledger-api. @KtorZ What would you prefer?

[KtorZ]

If the function isn't meant to be used then drop it 😄!

People/client consumers shouldn't need to have intrinsic knowledge of such subtleties, especially if there's another way to achieve the same operation but in a correct way. If it's available through the module API without warnings, I think it's reasonable to assume the function works in the most obvious way.

[KtorZ]

Note: you might want to notify maintainers of cardano-api, cardano-db-sync and hydra (possibly other) because it seems like everyone is relying on that function:

https://github.com/input-output-hk/cardano-api/blob/8561b7461957abd997020dcd7a603a4b804453aa/cardano-api/internal/Cardano/Api/Address.hs#L238

https://github.com/input-output-hk/cardano-db-sync/blob/86f41634dcfc38d669e2f188ecddeaae425a7bef/cardano-db-sync/src/Cardano/DbSync/Util/Address.hs#L36

https://github.com/input-output-hk/hydra/blob/60aac8c8718ffb069cd3a38c8baf88199e477de2/hydra-node/src/Hydra/Ledger/Cardano/Json.hs#L86

Honestly, when a function from a module API that has been working fine for years suddenly stop working after a change, I don't think the change can be called "a fix". When an undesired behavior is used and permanently stored in an immutable record, it becomes -- unfortunately -- a feature.

Also, it is confusing that the expected function to be used is called fromCborAddr and rely on a CBOR Decoder when addresses are arguably the only thing on the binary format that is not CBOR-encoded 😕 ...
As a consequence of that last point, it is necessary to forcefully serialize an address as CBOR-bytes first before being able to decode it. It seems that this happens in the ledger when turning Addr into CompactAddr (here -> https://github.com/input-output-hk/cardano-ledger/blob/master/libs/cardano-ledger-core/src/Cardano/Ledger/Address.hs#L408-L410). But that means client APIs are now expected to do an extra CBOR serialization before attempting to deserialize an address, for the sole purpose of annotating bytes as ... bytes.

[lehins]

Fair enough. We'll get it fixed.

[lehins]

I encourage you to checkout this PR with a fix: #3903 This being the most relevant commit for this issue: 31d15f1

you might want to notify maintainers of cardano-api, cardano-db-sync and hydra (possibly other) because it seems like everyone is relying on that function:

I've deprecated deserializeAddr, so they'll get notified.

Honestly, when a function from a module API that has been working fine for years suddenly stop working after a change, I don't think the change can be called "a fix".

I do agree that we should not have changed semantics of that function silently. Note, that it is not the first time that function changed behavior, prior to Alonzo it had another bug where it did not fail when input was not fully consumed. You must realize that Ledger's most important client is cardano-node and our top priority is making sure that data that goes on chain is valid and safe. In that respect that function had nasty bugs. The fact that ledger internals can often be used as a library for interfacing with data on chain is a nice byproduct. We are of course trying to make ledger more usable for all sorts of downstream users, that was the whole goal behind introduction of cardano-ledger-api (still largely incomplete). We have hundreds of functions implemented for correct Ledger operation, but that does not mean that all of them are stable for user consumption and will never change. That being said, whatever we export from cardano-ledger-api we will try to be extra careful in making sure that functionality is stable for the end user. So, feel free to request any functionality to be exported from that package in a form of an issue or a PR, if you feel like it could be useful. I can't guarantee that we will export it, because many things are really meant to be internal to ledger, but we'll definitely consider it.

it is confusing that the expected function to be used is called fromCborAddr and rely on a CBOR Decoder when addresses are arguably the only thing on the binary format that is not CBOR-encoded

There is nothing confusing about it, decodeAddr and fromCborAddr are two totally different functions and I would never suggest anyone use fromCborAddr in a manner you suggested. It just so happen that they bare some similarities, but they are intended for two separate purposes. fromCborAddr is designed to decode an address in CBOR that is part of a transaction and is protocol version aware. While decodeAddr has no relation to CBOR whatsover.

As a consequence of that last point, it is necessary to forcefully serialize an address as CBOR-bytes first before being able to decode it. It seems that this happens in the ledger when turning Addr into CompactAddr (here

That is not what happens there. This is a valid property for all valid addresses: deserialiseAddr (serialiseAddr addr) == addr, no CBOR involved.

People/client consumers shouldn't need to have intrinsic knowledge of such subtleties, especially if there's another way to achieve the same operation but in a correct way. If it's available through the module API without warnings, I think it's reasonable to assume the function works in the most obvious way.

Most obvious way for you is not necessarily most obvious way for another user. You are looking at it from the perspective of a use case that is relevant for your application, which is totally understandable. But look at at this function through the lens of someone who implements functionality for building or signing a transaction. In such scenario you do want the decoder to fail on an invalid addresses, because transaction with such an address will not be decoded by the node.

Because of different usage scenarios I prefer to go the safe route and not assume that anything is obvious. I've added two functions that can do lenient decoding of all types of addresses and support both bugs that we've discovered and fixed with bunch of documentation. Feel free to choose the one that works for you 😉

[KtorZ]

I've deprecated deserializeAddr, so they'll get notified.

🙏

You must realize that Ledger's most important client is cardano-node and our top priority is making sure that data that goes on chain is valid and safe

I do realize that, and I agree. Which is why I am insisting so much. These addresses ARE on chain already. So they are by definition valid. Or at least, have been valid at some point in time. It is more than reasonable to expect that the only function given by the ledger to decode an address would be able to decode any kind of address that the ledger (or any application consuming the blockchain) may encounter.

There is nothing confusing about it, decodeAddr and fromCborAddr are two totally different functions and I would never suggest anyone use fromCborAddr in a manner you suggested. It just so happen that they bare some similarities, but they are intended for two separate purposes. fromCborAddr is designed to decode an address in CBOR that is part of a transaction and is protocol version aware.

Common 🙃 ... Addresses are encoded as raw bytes in CBOR. For example, the address I shared earlier is encoded as:

58 26                                   # bytes(38)
   412813B99A80CFB4024374BD0F502959485AA56E0648564FF805F2E51BBCD9819561BDDC6614 

Thus, fromCborAddr is then the composition of (1) decoding CBOR bytes and (2) decoding the address payload which is a custom (not CBOR) binary encoding. At least, I am arguing that it ought to be. Yet right now, fromCborAddr does take care of selecting two different decoders based on the era, but only the second decoder is made available to downstream clients which creates confusion.

That is not what happens there. This is a valid property for all valid addresses: deserialiseAddr (serialiseAddr addr) == addr, no CBOR involved.

serialise & deserialise do involve CBOR! The address is binary format is described here: https://github.com/cardano-foundation/CIPs/blob/master/CIP-0019/CIP-0019-cardano-addresses.abnf (which now need adjustments since the semantic of POINTER has changed). What serialise does is simply to encode as CBOR-byte (Major Type 2) a custom binary payload for addresses which has, indeed, nothing to do with CBOR.

So what is confusing with fromCborAddr is that the function does both steps of decoding a CBOR bytestring, AND THEN decoding that bytestring which follows a custom, non-CBOR format.

Most obvious way for you is not necessarily most obvious way for another user. You are looking at it from the perspective of a use case that is relevant for your application, which is totally understandable.

No, I am looking at it from the perspective of ANY consumer of the cardano-ledger as a library. As stated earlier:

It is more than reasonable to expect that the only function given by the ledger to 
decode an address would be able to decode any kind of address that the ledger
(or any application consuming the blockchain) may encounter on the blockchain. 

[lehins]

It is more than reasonable to expect that the only function given by the ledger to
decode an address would be able to decode any kind of address that the ledger
(or any application consuming the blockchain) may encounter on the blockchain.

@KtorZ, it is not possible to export "the only" function like that, because it would not be correct for all use cases. We can export a function like that and that is what decodeAddrLenient is. Hopefully that satisfies your use cases. Alternatively we could not export any function that deserializes flat addresses that are not part of CBOR at all and let users deal with that problem on their own.

The key part in your statement is "or any application consuming the blockchain". Which is not every user of cardano-ledger! cardano-node is a user of ledger, as well as cardano-cli is a user of ledger and both of them cannot use such function, because they do not consume, but they build the blockchain! They only consume addresses as part of lager CBOR objects, which is era aware. So you are not thinking of ANY consumer. With respect to use cases that you linked to:

• db-sync only uses it for implementing a single irrelevant test case
• hydra does not use the function at all and the link you provided is to a piece of dead code
• cardano-api on the other is the only one that could use a function that does lenient decoding, because it implements functionality for querying the chain and bad addresses can be given as valid arguments, but it can be using the same decoder for transaction building interface, because transactions with bad addresses can no longer be placed on chain.

So they are by definition valid.

Wrong! Validity definition of an address very much depends on the context the address being used in. Addresses are only considered valid for the era they are being placed on chain! For example, would you still expect addresses that contain JPEG images be included on chain? Whenever you are trying to read from the chain or query the ledger state that is a totally different context! So, please do not mix up the two!

serialise & deserialise do involve CBOR!

But we are not using serialise and deserialise. We are not even using serialise package for anything in ledger except few places in testing, which I assume that was what you were referring to.

So what is confusing with fromCborAddr is that the function does both steps of decoding a CBOR bytestring, AND THEN decoding that bytestring which follows a custom, non-CBOR format.

Confusing as it may, that is the correct CBOR decoding. That is the function used for DecCBOR instance, which is used for deserializing addresses in a transaction. How else would you decode a binary blob in a CBOR object that contains a custom binary format?

Yet right now, fromCborAddr does take care of selecting two different decoders based on the era, but only the second decoder is made available to downstream clients which creates confusion.

decodeAddrLenient and decodeAddrLenientEither are the functions that being added to handle the other branches. Is that not sufficient for you? Could you describe clearly what you need and we can try to accommodate it. Changing semantics of decodeAddr cannot be that. I do want to avoid things like what happened to deserialiseAddr, which as I admitted before, we should not have done, thus the deprecation.

[KtorZ]

I give up 🤷 ... Do as you please. I'll adapt.

[lehins]

I give up 🤷 ... Do as you please

That's unfortunate. I was trying to reach some consensus here.