Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rpc): Refactor the serialization of note commitment trees #8533

Merged
merged 31 commits into from
May 22, 2024

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented May 14, 2024

Motivation

Close #8471.

The bug described in #8471 is caused by an incorrect serialization of empty note commitment trees for RPCs. Zebra serializes such trees as the empty string, but zcashd serializes them as 000000. This bug is related only to the z_get_treestate RPC. The state contains the right trees and uses a different serialization format.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

Solution

  • Remove the old RPC serialization code.

  • Use an upstream code to serialize the trees for RPCs.

    Note that the serialization of non-empty Sapling treestates now differs between zebrad and zcashd. We can see this in the results of the manual tests below. The difference is that zebrad's non-empty Sapling treestates contain trailing zeros, whereas zcashd's ones don't. Both serializations deserialize to the same frontier and use the sparse format, which permits the omission or inclusion of the trailing zeros. See https://discord.com/channels/809218587167293450/809251012610752513/1240372877358792755 for more details.

  • Cleanups.

Testing

Snapshots

  • Remove old test snapshots and add new ones.

Results of manual tests

The results below compare the responses from z_gettreestate to zcashd. Note that zebrad doesn't provide the finalState object, and when an object is empty, zebrad skips its serialization.

Sapling

zcash-cli z_gettreestate 419199 | jq

zebrad:

{
  "hash": "00000000025c3b19eb08bbc0d74c0c5e798fcd58b38ecdcdda6b83e5c5945295",
  "height": 419199,
  "time": 1540779316
}

zcashd:

{
  "hash": "00000000025c3b19eb08bbc0d74c0c5e798fcd58b38ecdcdda6b83e5c5945295",
  "height": 419199,
  "time": 1540779316,
  "sapling": {
    "commitments": {
      "finalRoot": "0000000000000000000000000000000000000000000000000000000000000000"
    }
  },
  "orchard": {
    "commitments": {
      "finalRoot": "0000000000000000000000000000000000000000000000000000000000000000"
    }
  }
}

zcash-cli z_gettreestate 419200 | jq

zebrad:

{
  "hash": "00000000025a57200d898ac7f21e26bf29028bbe96ec46e05b2c17cc9db9e4f3",
  "height": 419200,
  "time": 1540779337,
  "sapling": {
    "commitments": {
      "finalState": "000000"
    }
  }
}

zcashd:

{
  "hash": "00000000025a57200d898ac7f21e26bf29028bbe96ec46e05b2c17cc9db9e4f3",
  "height": 419200,
  "time": 1540779337,
  "sapling": {
    "commitments": {
      "finalRoot": "3e49b5f954aa9d3545bc6c37744661eea48d7c34e3000d82b7f0010c30f4c2fb",
      "finalState": "000000"
    }
  },
  "orchard": {
    "commitments": {
      "finalRoot": "0000000000000000000000000000000000000000000000000000000000000000"
    }
  }
}

zcash-cli z_gettreestate 419201 | jq

zebrad:

{
  "hash": "00000000014d117faa2ea701b24261d364a6c6a62e5bc4bc27335eb9b3c1e2a8",
  "height": 419201,
  "time": 1540779438,
  "sapling": {
    "commitments": {
      "finalState": "019eb30778ddeea84c72e69e07a1689f3c8def3dc0a1939f0edcbe47279069d931001f000150715810d52caf35471d10feb487213fbd95ff209122225b7b65d27a7fb1a44d0000000000000000000000000000000000000000000000000000000000"
    }
  }
}

zcashd:

{
  "hash": "00000000014d117faa2ea701b24261d364a6c6a62e5bc4bc27335eb9b3c1e2a8",
  "height": 419201,
  "time": 1540779438,
  "sapling": {
    "commitments": {
      "finalRoot": "638d7e5ba37ab7921c51a4f3ae1b32d71c605a0ed9be7477928111a637f7421b",
      "finalState": "019eb30778ddeea84c72e69e07a1689f3c8def3dc0a1939f0edcbe47279069d9310002000150715810d52caf35471d10feb487213fbd95ff209122225b7b65d27a7fb1a44d"
    }
  },
  "orchard": {
    "commitments": {
      "finalRoot": "0000000000000000000000000000000000000000000000000000000000000000"
    }
  }
}

zcash-cli z_gettreestate 1687104 | jq

zebrad:

{
  "hash": "0000000000d723156d9b65ffcf4984da7a19675ed7e2f06d9e5d5188af087bf8",
  "height": 1687104,
  "time": 1654019405,
  "sapling": {
    "commitments": {
      "finalState": "01b44a49543697efcf8c8f31759426c7baaea332c188a5ad2cbe0f7cdc35f5ad3f001f013cef92c19a6d24ba2e4454d018f7c7d6a64e0d7dc428a7a463617c726459230f01e8fe3507e2380ce4b17febf0bd5692883b5eff1cf9336c94fbec2153ece9e752014985ea98e1e8612a883112e794599176c1d84e5eccf531aecf7ecb0630a3ed1d01c1eacd8226ff8865f6c2495cb6c69f3e2fb71f142264dd123cad696ac4e3d35f015addd80e5152fbd8eeb49177dd46a6626b1629d4ed31a833155ca3b6cf3cb54101279f7afb4866502d697c5cbbd4a2b86bfb981dbe3c5ef29538ad1234159d3673010284d6898ca14c204611c5549bd410e6524053af5a166a6c5837653f46c3074501ae61a98ec05d976a41e46f0b7ae8dee5bda5ab490eea7b701153511a15be42590198c748a5e9516711db607e282f08013ba00b3e77bc66ec28702ca17893a5154b000139113a1e0f54d93c6180f2df7d16afbf7c320f6c7665cb10e1fc309878b0d716019a0214bcb1fd7a70e53166101992147512f3debb0fdce45ac625fffd64771010000134136b9f1f00c2e9d16dc7358ef920862511c8fc42fb7074cbc9016d8d4e8b4c015eddc191a81221b7900bbdcd8610e5df595e3cdc7fd5b3432e3825206ae35b05017eda713cd733ccc555123788692a1876f9ca292b0aa2ddf3f45ed2b47f027340000000015ec9e9b1295908beed437df4126032ca57ada8e3ebb67067cd22a73c79a840090000000000000000000000"
    }
  },
  "orchard": {
    "commitments": {
      "finalState": "000000"
    }
  }
}

zcashd:

{
  "hash": "0000000000d723156d9b65ffcf4984da7a19675ed7e2f06d9e5d5188af087bf8",
  "height": 1687104,
  "time": 1654019405,
  "sapling": {
    "commitments": {
      "finalRoot": "4c0455466849dafedc0aab00f842f5ce4ef7dc1e795995d460fd072ce20f609f",
      "finalState": "01b44a49543697efcf8c8f31759426c7baaea332c188a5ad2cbe0f7cdc35f5ad3f0014013cef92c19a6d24ba2e4454d018f7c7d6a64e0d7dc428a7a463617c726459230f01e8fe3507e2380ce4b17febf0bd5692883b5eff1cf9336c94fbec2153ece9e752014985ea98e1e8612a883112e794599176c1d84e5eccf531aecf7ecb0630a3ed1d01c1eacd8226ff8865f6c2495cb6c69f3e2fb71f142264dd123cad696ac4e3d35f015addd80e5152fbd8eeb49177dd46a6626b1629d4ed31a833155ca3b6cf3cb54101279f7afb4866502d697c5cbbd4a2b86bfb981dbe3c5ef29538ad1234159d3673010284d6898ca14c204611c5549bd410e6524053af5a166a6c5837653f46c3074501ae61a98ec05d976a41e46f0b7ae8dee5bda5ab490eea7b701153511a15be42590198c748a5e9516711db607e282f08013ba00b3e77bc66ec28702ca17893a5154b000139113a1e0f54d93c6180f2df7d16afbf7c320f6c7665cb10e1fc309878b0d716019a0214bcb1fd7a70e53166101992147512f3debb0fdce45ac625fffd64771010000134136b9f1f00c2e9d16dc7358ef920862511c8fc42fb7074cbc9016d8d4e8b4c015eddc191a81221b7900bbdcd8610e5df595e3cdc7fd5b3432e3825206ae35b05017eda713cd733ccc555123788692a1876f9ca292b0aa2ddf3f45ed2b47f027340000000015ec9e9b1295908beed437df4126032ca57ada8e3ebb67067cd22a73c79a84009"
    }
  },
  "orchard": {
    "commitments": {
      "finalRoot": "ae2935f1dfd8a24aed7c70df7de3a668eb7a49b1319880dde2bbd9031ae5d82f",
      "finalState": "000000"
    }
  }
}

zcash-cli z_gettreestate 1687107 | jq

zebrad:

{
  "hash": "00000000005a5e6f54c494b6317f3e800ce89a716584e62dcb35c2b3ace4b498",
  "height": 1687107,
  "time": 1654019794,
  "sapling": {
    "commitments": {
      "finalState": "01548d76c9708e1ea19cbfca3a0da6e368210dbe36ed022f32641416070760e750001f01a122025ebf64510a565e95a48604ff431c5cde6cb8caf9b7234066fcd1325c2a01162d1c2490d6bac0c7622d7a6aa214959367513dc2c45711a0d386bb3aafd261000000000000000138ebc51a133a60cdd2c02bdef27df1f1aa68882e0550a948fd7fb6f6b71e07210139113a1e0f54d93c6180f2df7d16afbf7c320f6c7665cb10e1fc309878b0d716019a0214bcb1fd7a70e53166101992147512f3debb0fdce45ac625fffd64771010000134136b9f1f00c2e9d16dc7358ef920862511c8fc42fb7074cbc9016d8d4e8b4c015eddc191a81221b7900bbdcd8610e5df595e3cdc7fd5b3432e3825206ae35b05017eda713cd733ccc555123788692a1876f9ca292b0aa2ddf3f45ed2b47f027340000000015ec9e9b1295908beed437df4126032ca57ada8e3ebb67067cd22a73c79a840090000000000000000000000"
    }
  },
  "orchard": {
    "commitments": {
      "finalState": "01e542b41a8a44e417521228218da39f865283ae50431c2292c36f379f6da04d2d0138fb218b939d9d6b7e906f1e68e49b4a5b9c1941fbf21c543529b19eadbcb01f1f00000000000000000000000000000000000000000000000000000000000000"
    }
  }
}

zcashd:

{
  "hash": "00000000005a5e6f54c494b6317f3e800ce89a716584e62dcb35c2b3ace4b498",
  "height": 1687107,
  "time": 1654019794,
  "sapling": {
    "commitments": {
      "finalRoot": "4b013ee86a4a7b19871aa9b7a1dff0b29e0a737812c8970356c18340752957c7",
      "finalState": "01548d76c9708e1ea19cbfca3a0da6e368210dbe36ed022f32641416070760e750001401a122025ebf64510a565e95a48604ff431c5cde6cb8caf9b7234066fcd1325c2a01162d1c2490d6bac0c7622d7a6aa214959367513dc2c45711a0d386bb3aafd261000000000000000138ebc51a133a60cdd2c02bdef27df1f1aa68882e0550a948fd7fb6f6b71e07210139113a1e0f54d93c6180f2df7d16afbf7c320f6c7665cb10e1fc309878b0d716019a0214bcb1fd7a70e53166101992147512f3debb0fdce45ac625fffd64771010000134136b9f1f00c2e9d16dc7358ef920862511c8fc42fb7074cbc9016d8d4e8b4c015eddc191a81221b7900bbdcd8610e5df595e3cdc7fd5b3432e3825206ae35b05017eda713cd733ccc555123788692a1876f9ca292b0aa2ddf3f45ed2b47f027340000000015ec9e9b1295908beed437df4126032ca57ada8e3ebb67067cd22a73c79a84009"
    }
  },
  "orchard": {
    "commitments": {
      "finalRoot": "7b61fc613cea5c2c84c5e2c64d4fd4afb8c8c9d10dce9bcad49431c9cf32f131",
      "finalState": "01e542b41a8a44e417521228218da39f865283ae50431c2292c36f379f6da04d2d0138fb218b939d9d6b7e906f1e68e49b4a5b9c1941fbf21c543529b19eadbcb01f1f00000000000000000000000000000000000000000000000000000000000000"
    }
  }
}

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@upbqdn upbqdn added C-bug Category: This is a bug I-usability Zebra is hard to understand or use A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡ rust Pull requests that update Rust code labels May 14, 2024
@upbqdn upbqdn self-assigned this May 14, 2024
@upbqdn upbqdn marked this pull request as ready for review May 15, 2024 18:56
@upbqdn upbqdn requested review from a team as code owners May 15, 2024 18:56
@upbqdn upbqdn requested review from arya2 and removed request for a team May 15, 2024 18:56
mergify bot added a commit that referenced this pull request May 15, 2024
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
Co-authored-by: Arya <aryasolhi@gmail.com>
mergify bot added a commit that referenced this pull request May 16, 2024
oxarbitrage
oxarbitrage previously approved these changes May 20, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, i think it looks great. I love the amount of code we were able to remove from Zebra here.

zebra-rpc/Cargo.toml Show resolved Hide resolved
Cargo.lock Show resolved Hide resolved
mergify bot added a commit that referenced this pull request May 20, 2024
mergify bot added a commit that referenced this pull request May 22, 2024
@oxarbitrage
Copy link
Contributor

@Mergifyio refresh

Copy link
Contributor

mergify bot commented May 22, 2024

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request May 22, 2024
@mergify mergify bot merged commit eade2a8 into main May 22, 2024
159 checks passed
@mergify mergify bot deleted the fix-z-gettreestate branch May 22, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug I-usability Zebra is hard to understand or use P-Medium ⚡ rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: get_tree_state fails at sapling activation height + 0 and +1
3 participants