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

Synchronizer.proposeShielding crashes when there are insufficient funds to shield #1420

Open
str4d opened this issue Apr 30, 2024 · 1 comment · Fixed by #1424
Open

Synchronizer.proposeShielding crashes when there are insufficient funds to shield #1420

str4d opened this issue Apr 30, 2024 · 1 comment · Fixed by #1424
Labels
bug Something isn't working

Comments

@str4d
Copy link
Collaborator

str4d commented Apr 30, 2024

In #1382 (specifically in 84ac625) we adjusted Synchronizer.proposeShielding to return null when there are either no funds to shield, or they do not meet the shielding threshold (equivalent to the Rust type Option<Proposal>). However this only altered the API, as the FFI still always created a proposal; the backend changes need to be bundled together separately into binary releases of the -ffi backend.

When the -ffi backend was updated to enable zcashlc_propose_shielding to return an optional proposal, the FFI representation was not hooked up to ZcashRustBackend.proposeShielding at all.

The problem is in this code:

@DBActor
func proposeShielding(
account: Int32,
memo: MemoBytes?,
shieldingThreshold: Zatoshi,
transparentReceiver: String?
) async throws -> FfiProposal? {

return try FfiProposal(contiguousBytes: Data(
bytes: proposal.pointee.ptr,
count: Int(proposal.pointee.len)
))
}

This attempts to construct a Data from a null pointer, when it should instead check for nullability and then either return null, or construct the Data from the known-non-null pointer.

The bug has been observed very infrequently, most likely because wallets generally only try to shield when there are any funds at all, and in that case the funds are most likely non-trivial in value (and thus above the shielding threshold).

@str4d str4d added the bug Something isn't working label Apr 30, 2024
LukasKorba added a commit to LukasKorba/ZcashLightClientKit that referenced this issue May 14, 2024
…en there are insufficient funds to shield

- Check for the null value of the pointer when constructing propose shielding
LukasKorba added a commit to LukasKorba/ZcashLightClientKit that referenced this issue May 14, 2024
…en there are insufficient funds to shield

- Check for the null value of the pointer when constructing propose shielding

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield

- throw an error instead of returning nil value
LukasKorba added a commit to LukasKorba/ZcashLightClientKit that referenced this issue May 14, 2024
…en there are insufficient funds to shield

- Check for the null value of the pointer when constructing propose shielding

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield

- throw an error instead of returning nil value

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield

- Changelog updated
- Error message polished a bit
LukasKorba added a commit to LukasKorba/ZcashLightClientKit that referenced this issue May 15, 2024
…en there are insufficient funds to shield

- Check for the null value of the pointer when constructing propose shielding

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield

- throw an error instead of returning nil value

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield

- Changelog updated
- Error message polished a bit

[Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficien… …t funds to shield  - Check for the null value of the pointer when constructing propose shielding  [Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield  - throw an error instead of returning nil value  [Electric-Coin-Company#1420] Synchronizer.proposeShielding crashes when there are insufficient funds to shield (Electric-Coin-Company#1424)

- Refactored the error msg a bit
LukasKorba added a commit that referenced this issue May 15, 2024
…elding-crashes-when-there-are-insufficient-funds-to-shield

[#1420] Synchronizer.proposeShielding crashes when there are insuffic…
@str4d
Copy link
Collaborator Author

str4d commented Jul 9, 2024

#1424 incorrectly resolved this issue. It throws an error when a nil proposal is encountered, instead of returning nil. This results in an error being shown to the user, when instead the wallet should get from the SDK a nil proposal and interpret that as "there are insufficient funds to meet the shielding threshold".

@str4d str4d reopened this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant