Minor bug: the SDK throws a runtime error instead of returning details about a failure to connect to orderers

[Quasso]

Which SDK version am I using?

v2.2.3

Details of the issue

Bug occurs due to a hard check on an object property failing in cases where commitResponse is undefined.

• This applies to the file ./fabric-network/src/transaction.ts on lines 338 & 339 (commitResponse.status throws a hard error where commitResponse is undefined)

Proposed fix

I have already monkey-patched the compiled file transaction.js (installed under the directory /usr/local/lib/node_modules/fabric-network) inside my Kuberneters network and confirmed that in this instance where the orderers are unable to connect (actually whenever commitRepsonse is undefined) it returns a useful altnerative error which is subsequently thrown as expected, for example, when running a Caliper test which uses the transpiled SDK to submit transactions.

I am happy to submit this fix as a PR with an included test as per @davidkel's suggestion (this will take a little longer since it requires me getting everything building locally to get the test run). I have an up-to-date fork of the repository (parity with branch main) on which I'll do this on and will submit the PR during this week (probably the next couple of days!)

[bestbeforetoday]

This is not a condition that is intended to occur, so not something I think we should be adding code to handle. The problem is that the the commit send() is returning an undefined value.

Can you give any more details on the conditions where you see this error? Are you specifying endorsing peers for the transaction? Is discovery enabled? Are you specifying endorsing organisations for the transaction?

Or perhaps better, attaching a debug log of the failing invocation should answer the questions above and might provide some other insights.

[Quasso]

thanks for your reply @bestbeforetoday. In my view, given the nature of the failure on my side this could potentially happen for a multitude of reasons, where connectivity to the network fails partially (gateway is instantiated but peers/orderers can't connect).

As it happens, in my context it was to do with using Caliper for testing, where I had set the configuration for fabric to use localhost: false in the network config YAML, but this setting was ignored. It meant that the gateway connection established properly, tests began running (to create transactions) and then it failed to connect to peers/orderers (my setup is a Kubernetes network, requiring hostname resolution).

I spent ages debugging this because a runtime error was thrown by the SDK (no exception was caught, due to the code in question) and ultimately had to monkey-patch the transaction.js file with aforementioned fix to provide a contextual error message, and allow other error messages to be returned in the terminal (which led to a fix).

I filed an issue for this over on the Caliper repo, you can see the related info here which should provide extra context: https://github.com/hyperledger/caliper/issues/1216.

Hope this helps!

[bestbeforetoday]

@davidkel I'm not familiar enough with Caliper to obviously spot the SDK parameters used for transaction invocation based on the Caliper configuration in hyperledger/caliper#1216. The only path I can obviously spot where an undefined result could be returned from the send to the orderer with discovery enabled is if specific target peers are specified for endorsement and the channel returns no orderers. Could that be the case here?

https://github.com/hyperledger/fabric-sdk-node/blob/14a901ad0f4ed5dfee9c2d325f8b3bbbf500fbba/fabric-network/src/transaction.ts#L334

[Quasso]

@davidkel I'm not familiar enough with Caliper to obviously spot the SDK parameters used for transaction invocation based on the Caliper configuration in hyperledger/caliper#1216. The only path I can obviously spot where an undefined result could be returned from the send to the orderer with discovery enabled is if specific target peers are specified for endorsement and the channel returns no orderers. Could that be the case here?

https://github.com/hyperledger/fabric-sdk-node/blob/14a901ad0f4ed5dfee9c2d325f8b3bbbf500fbba/fabric-network/src/transaction.ts#L334

Just curious, did you see the draft PR I made which addresses this? https://github.com/hyperledger/fabric-sdk-node/pull/549 (I guess you did but since you're mentioning this line!)

The lines I modified are right after that, and they perform the necessary check of whether commitResponse is undefined and handle it. It's by no means ready to merge but I just wanted to throw down the general idea in TS which worked for me on the running JS I monkey-patched. I am happy to get the env running properly to get this to build correctly with an appropriate test if you decide it's worth doing :)

[bestbeforetoday]

Yes, I saw the PR. It is certainly an effective sticking plaster to cover up the underlying problem. If possible, I'd prefer to fix the underlying problem which, as I mentioned above, is that commit.send() is returning an undefined value when it shouldn't.

[Quasso]

Sorry my misunderstanding, thanks for clarifying. I hear what you're saying... I could have a look into this myself this week if you like, see if I can help with that instead. I think I took what David said quite literally and proposed this without thinking so much about the underlying... that sounds like an interesting thing to look at!

[davidkel]

@Quasso As @bestbeforetoday has pointed to I think the problem stems from this line of code here https://github.com/hyperledger/fabric-sdk-node/blob/14a901ad0f4ed5dfee9c2d325f8b3bbbf500fbba/fabric-common/lib/Commit.js#L182 If the call returns an empty set of committers (and in your scenario I'm guessing it does) then the send method is going to return undefined and the subsequent code will result in a type error.

[Quasso]

@davidkel thanks yeah I hadn't taken the time to trace it back but that seems very reasonable. I will delete my other PR in this case and happy to submit another soon to take care of that if you guys are happy with that!

[bestbeforetoday]

@Quasso If you fancy submitting a PR, that would be great. I would suggest starting with a unit test that reproduces the condition and fails because the return value is undefined instead of the expected value. The logic in that send() method looks like it could probably be simplified a bit, but I guess the key thing is that it should throw an error instead of returning undefined if there are no committers.

[Quasso]

@bestbeforetoday great stuff, I have always wanted to contribute to open source projects which I use, but have never really found the right occasion/thing to address, along with having the bandwidth to do it.

I'm a big fan of the Hyperledger outfit in general and this SDK in particular is powerful & really helpful if you're wanting to build a heavily customised Fabric network with as much technical overlap as possible (between the API, admin app, chaincodes etc) while relying on the more popularly practiced & understood TypeScript (or JS) than for example Go.

In conclusion, I am:

• grateful to you & all the other contributors to this project
• pretty delighted to be able to offer something back!

With that said, I will endeavour to submit a PR hopefully this week, but definitely as soon as I can. And thank you, appreciate the pointers... I will follow those for sure.