Fixed bug: For extension node condition should use current path pointer

[0xsarvesh]

Fixes #652

[schemar]

Nice one. I think it can be improved to increase future readability, though. In case we come to this place again.
Also, does the git commit explain what the issue and the fix are? If not you could amend your commit with a better message and force push that to your branch.

[benjaminbollen]

I added some thoughts to the fix; do they make sense?

[schemar]

I just want to restate that we also need a lot of positive and negative test cases. Can we add some? Generally, any bug fix should come with at least the unit test that fails due to the bug and passes after the fix.

[0xsarvesh]

I just want to restate that we also need a lot of positive and negative test cases. Can we add some? Generally, any bug fix should come with at least the unit test that fails due to the bug and passes after the fix.

I can easily add tests for the cases which kit reported. 👍

[0xsarvesh]

@benjaminbollen and @schemar
I did suggested changes which also includes tests.
Please have a look to check if I haven't missed anything. 🙏

[schemar]

Nice! However, can we also add tests where verification should fail and assert that they still fail with the new code? I am not sure how much we want to test this, but from my point of view 2 tests seems a little slim.

Also minor in-line comments.

[benjaminbollen]

LGTM

[schemar]

LGTM if we don't want to add more tests 👍

[benjaminbollen]

I propose two paths:

1. we can keep this hotfix on the branch, and over the coming days we put effort to adding more explicit unit tests
2. we merge it with release-0.10, develop and publish beta.3 and we put effort to add more explicit unit tests 😝

[0xsarvesh]

Nice! However, can we also add tests where verification should fail and assert that they still fail with the new code? I am not sure how much we want to test this, but from my point of view 2 tests seems a little slim.

Also minor in-line comments.

@schemar These two tests are the scenarios where the bug was reported. It makes sense to add these tests that give confidence that the fix works.

However, what you are requesting is to test the MPP library in general, which I would say should be done as a separate ticket. Testing MPP library for all the scenarios is a significant effort, as we don't have proof for all the possible trie trees. These need to be manually formed(Unless there is a better alternative)

I am not sure if we want to do this for this fix.
@benjaminbollen what's your call?

[benjaminbollen]

@sarvesh-ost yes, I think this hotfix PR should go ahead so that we can test it, both in KIT and in JLP examples;

to @schemar s point, yes let's put focus on additional deliberate tests, but this is more work than this issue (echoing Sarvesh's point)

I propose we merge this into the hot fix branch; and go forward with publishing beta.3 so we can test it from KIT and JLP and increase coverage on develop

[schemar]

LGTM in light of #653