search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
896 stars 198 forks source link

Fix doc and double save in client #155

Closed mszulcz-mitre closed 2 years ago

mszulcz-mitre commented 2 years ago

This PR contains 2 commits:

1. The 1st commit fixes an inaccurate method description

Here's the description of 'client::init': https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/client/client.hpp#L37-L42

The implementation shows that init does not attempt to create data files if they don't exist: https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/client/client.cpp#L33-L48

The creation of a wallet data file occurs in transaction::wallet::save: https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/transaction/wallet.cpp#L307-L327

The creation of a client data file occurs in client::save_client_state: https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/client/client.cpp#L287-L298

2. The 2nd commit removes a redundant call

client::mint appears to call save twice. As a result, the files written in the 1st call are cleared and the same data is then re-written to them. The first call to save occurs when mint calls import_transaction: https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/client/client.cpp#L57-L70 https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/client/client.cpp#L198-L201 The 2nd call occurs in mint itself (line 67). This commit deletes the 2nd call.

mszulcz-mitre commented 2 years ago

Line 312 in wallet.cpp has a TODO: https://github.com/mit-dci/opencbdc-tx/blob/2b21cc9f857cabaf129a0a413a3ed8825dea2f36/src/uhs/transaction/wallet.cpp#L307-L316

Is there any reason not to follow up on it?

mszulcz-mitre commented 2 years ago

@HalosGhost For my last few PRs, I've seen tests pass on my machine but fail here, and I've wondered if it's due to known issues in the test suite (e.g. Issue #127) or problems with my commits. Is there anything I can do to check? For example, is there a list of tests susceptible to the issues in #127 that I should expect to fail sometimes? Is there a way I can re-run the tests here (without adding new commits) to see if they pass on a 2nd or 3rd run?

HalosGhost commented 2 years ago

I've seen tests pass on my machine but fail here, and I've wondered if it's due to known issues in the test suite (e.g. Issue #127) or problems with my commits.

That they pass on re-run (or have in other PRs at least, the current rerun of unit/integration tests for this PR hasn't finished) strongly suggests it's fragility in the test suite rather than in your work.

Is there anything I can do to check? For example, is there a list of tests susceptible to the issues in #127 that I should expect to fail sometimes? Is there a way I can re-run the tests here (without adding new commits) to see if they pass on a 2nd or 3rd run?

You can certainly use a tool like act to run the CI locally a few times. We don't have specific documentation of which tests are prone to failure, but a good short-hand is “does the test include a call to sleep()?” as that's a significant part of why the test suite is a bit fragile.

As for re-running them, feel free to ping me on Zulip (or here); it's a single button-press for me to re-run one of the CI steps.

Yep, CI passed with flying colors on-rerun.

HalosGhost commented 2 years ago

So, the second save() comes following send_mint_tx(). As far as I can tell, this should be safe because the calls under send_mint_tx() which make modifications should all call save() themselves.

@metalicjames is there a specific reason to keep this (potentially) redundant call to save()?