hyperledger / fabric-chaincode-java

Hyperledger Fabric Contract and Chaincode implementation for Java
https://hyperledger.github.io/fabric-chaincode-java/
Apache License 2.0
304 stars 207 forks source link

Handling 404 in getPrivateData (ChaincodeStub) #214

Closed oborovyk closed 2 years ago

oborovyk commented 2 years ago

There's no way currently to distinguish between 404 (no data found) - possible use-case and 500 (error when trying to get data) when querying data from the private collection. There's a single error handling block that handles all the cases. The details are also not included in the RuntimeException being thrown which may lead to data corruption in the smart contract. Is there any workaround? Thanks

jt-nti commented 2 years ago

Can you provide more details of what you're trying to achieve and what's causing the RuntimeException to be thrown?

The ContractRuntimeException is not intended to be used by smart contract transactions directly and is only meant to handle internal problems, i.e. not something client apps are expected to be able to do anything about. I hope the cause does get logged though- is that not happening?

The idea is that smart contracts should handle any errors using ChaincodeException instead. The asset transfer samples do handle data not found for getStringState (and also include a payload** in the error). Would a similar approach work for your scenario?

https://github.com/hyperledger/fabric-samples/blob/main/asset-transfer-basic/chaincode-java/src/main/java/org/hyperledger/fabric/samples/assettransfer/AssetTransfer.java#L138

** unfortunately while the low level interfaces allow chaincode errors to include a payload, and Java chaincode can use the ChaincodeException to provide a payload, I don't think the client SDKs use it currently. There is an open issue to discuss what the best practices for handling errors in chaincode should be, which it would be really useful to get your views on

hyperledger/fabric-samples#543

Thanks, James

oborovyk commented 2 years ago

Hi @jt-nti . Thx for the hint and the discussion link. The case is similar to assetTransfer indeed. Check if the bucket (asset) is created and create it, if not. What we were seeing in the log was {"level":"error","ts":1638444247.390414,"name":"chaincode","caller":"chaincode/handler.go:251","msg":"[e5c06d29] Failed to handle GET_STATE. error: private data matching public hash version is not available. Public hash version = {BlockNum: 227, TxNum: 0}, Private data version = <nil> Which doesn't explicitly tell that the data is simply not there. (there was a message from couchdb somewhere in the other log with 404 but connecting these 2 was not easy) Temporary fix that works now

       try {
            var privateData = stub.getPrivateData(COLLECTION, key);
            return new State(COLLECTION, key, privateData);
        } catch (Exception e) {
            logger.warning("failed to get private data:" + e.getMessage());
            return new State(COLLECTION, key, new byte[0]);
        }

I'll try with the getStringState but imho the expected behavior for getPrivateData is to return the empty array in such cases.

jt-nti commented 2 years ago

The sample asset exists method checks for an empty result as well, so I think the same approach should work for getPrivateData, e.g. something like...

    @Transaction(intent = Transaction.TYPE.EVALUATE)
    public boolean PrivateDataExists(final Context ctx, final String key) {
        ChaincodeStub stub = ctx.getStub();
        byte[] data = stub.getPrivateData(COLLECTION, key);

        return (data != null && data.length > 0);
    }

And to return an error in other transactions...

        if (!PrivateDataExists(ctx, key)) {
            String errorMessage = String.format("Private data key %s does not exist", key);
            throw new ChaincodeException(errorMessage);
        }

The asset-transfer-private-data uses private data as well if that helps.

oborovyk commented 2 years ago

Just to confirm my understanding - the key element here is @Transaction(intent = Transaction.TYPE.EVALUATE) and annotating it like that will change the error handling from RuntimeException to an empty response?

jt-nti commented 2 years ago

Hi @oborovyk,

Just to confirm my understanding - the key element here is @transaction(intent = Transaction.TYPE.EVALUATE) and annotating it like that will change the error handling from RuntimeException to an empty response?

No, sorry for the confusion, the Transaction annotation is just for the contract metadata. There's a tutorial about contract metadata in the node chaincode project if you're interested.

The key element is using ChaincodeException to include a suitable error message response for client apps to find.

oborovyk commented 2 years ago

ok - than I'll try to clarify from my end Problem is we do not have control in the contract to get to the throw ChaincodeException. The shim itself is throwing runtime exception if data is missing meaning the next line will not complete byte[] data = stub.getPrivateData(COLLECTION, key); error in the log {“level”:“error”,“ts”:1638448415.9364188,“name”:“chaincode”,“caller”:“chaincode/handler.go:251”,“msg”:“[b4d0361c] Failed to handle GET_STATE. error: private data matching public hash version is not available. Public hash version = {BlockNum: 152, TxNum: 0}, Private data version = <nil>\ngithub.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/txmgr.(*queryExecutor).GetPrivateData\n\t/go/src/github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/txmgr/query_executor.go:217\ngithub.com/hyperledger/fabric/core/chaincode.(*Handler).HandleGetState\n\t/go/src/github.com/hyperledger/fabric/core/chaincode/handler.go:615\ngithub.com/hyperledger/fabric/core/chaincode.(*Handler).HandleTransaction\n\t/go/src/github.com/hyperledger/fabric/core/chaincode/handler.go:246\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1371\ngithub.com/hyperledger/fabric/core/chaincode.

jt-nti commented 2 years ago

Ah, thanks for the clarification, I see what you mean now. I don't think getPrivateData should be throwing a runtime exception for that. You should be able to work around it by putting getPrivateData in a try/catch (Exception e) block but it sounds like there's an issue with getPrivateData which needs investigating/fixing.

DavidScheers commented 2 years ago

Ah, thanks for the clarification, I see what you mean now. I don't think getPrivateData should be throwing a runtime exception for that. You should be able to work around it by putting getPrivateData in a try/catch (Exception e) block but it sounds like there's an issue with getPrivateData which needs investigating/fixing.

We currently implemented the work around with the try/catch. The issue with this is that we catch "everything" and not just the "absent data / not found". I would like to have some documentation for the getPrivateData method how it will return absent data. Will it return null? Will it return an empty byte[] ? Will it throw a specific exception ?

jt-nti commented 2 years ago

We currently implemented the work around with the try/catch. The issue with this is that we catch "everything" and not just the "absent data / not found".

It should be possible to check if the exception is related to the data not found case and rethrow other exceptions.

I would like to have some documentation for the getPrivateData method how it will return absent data. Will it return null? Will it return an empty byte[] ? Will it throw a specific exception ?

I've just been looking through the JavaDoc and it seems a bit vague about what is meant to happen. My guess is that it should return an empty byte[] for consistency with getState but I'll check. I think the JavaDoc needs updating either way.

DavidScheers commented 2 years ago

It should be possible to check if the exception is related to the data not found case and rethrow other exceptions.

If you look into ChaincodeInvocationTask invoke(), it seems all errors are RuntimeExceptions. And the message is either "Unsuccessful response" or "Unexpected response".

jt-nti commented 2 years ago

I've been looking through the peer code and it seems like the error you're seeing is a genuine a runtime error, not just a normal case of absent data:

https://github.com/hyperledger/fabric/blob/main/core/ledger/kvledger/txmgmt/txmgr/query_executor.go#L217-L220

Could there be a chaincode/collection configuration issue?

jt-nti commented 2 years ago

Apparently it means private data wasn't distributed to that peer collection member for a specific key and therefore the peer can't endorse future transactions for the key. Usually caused by not enough peer redundancy and dissemination at time of original endorsement. See https://hyperledger-fabric.readthedocs.io/en/latest/private-data-arch.html#private-data-collection-definition requiredPeerCount

oborovyk commented 2 years ago

Thanks a lot for the advice. Looks like indeed the issue was related to private collection dissemination across our 2 peers. (We were getting those errors from both peers - that's why this idea was discarded in the first place). For now, we are setting requiredPeerCount=1 and hope for the best (cause this is hard to reproduce). Is there maybe some tooling to check the private data collection integrity?

P.S. We can probably close this if there's another issue for improving the error handling doc.

jt-nti commented 2 years ago

Hi @oborovyk, there is now an issue to improve that error message which should hopefully help in the future -> hyperledger/fabric#3153

Tooling to check for missing private data sounds like a great idea. There's a new ledgerutil command which seems like a reasonable place to add something like that. I'll go ahead and close this issue but please open a Fabric issue if extending the ledgerutil command is something that you think would be useful.

Thanks, James