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

CLI wallet: avoid directly overwriting wallet file on exit #1109 #1195

Merged
merged 11 commits into from Aug 17, 2018

Conversation

Projects
4 participants
@cogutvalera
Copy link
Member

commented Jul 27, 2018

PR for #1109

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

The logic looks fine, although naming new file as ".backup" is not so good.

@pmconrad
Copy link
Contributor

left a comment

AFAICS fc::ofstream does not throw nor otherwise communicate failure on write(), flush() or close().

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

we should create another issue for fc::ofstream IMHO

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 28, 2018

AFAICS fc::ofstream does not throw nor otherwise communicate failure on write(), flush() or close().

It means we need to make sure the new (tmp) file is good before calling fc::rename, otherwise it's still possible to lose data. @cogutvalera creating a new issue is fine, but I think this issue/PR depends on the fix of it.

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_1109 branch from 9fa9461 to db840c8 Jul 28, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2018

review lost commits, now checking tmp wallet file contents to be equal as required data before renaming this tmp wallet file

}
else
{
disable_umask_protection();

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jul 29, 2018

Contributor

This is not necessary, because it already happens in the outer try/catch.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 29, 2018

Author Member

Why it is not necessary ? I've tested & checked it and we won't get into outer try/catch code after throwing exception here that's why I've placed disable_umask_protection

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jul 29, 2018

Contributor

Using throw without specifying an exception is only allowed in a catch clause, otherwise it will terminate instead of actually throwing, see below.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 29, 2018

Author Member

yes, you're absolutely right ! I've checked it earlier, that's why I told you that disable_umask_protection is necessary there before using throw, if I will change throw to fc::exception subtype then I will remove disable_umask_protection call. Just couldn't understand why you told that disable_umask_protection isn't necessary before using throw.

else
{
disable_umask_protection();
throw;

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jul 29, 2018

Contributor

Please throw an fc::exception subtype with an appropriate error message.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 29, 2018

Author Member

Then we also need to throw fc::exception subtype in outer try/catch block ? right ?

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jul 31, 2018

Contributor

no, the outer catch should re-throw whatever it caught.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 31, 2018

Author Member

yes sure, already done without any changes in outer try/catch block

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 29, 2018

@abitmore abitmore added this to In progress in Feature release (201810) via automation Jul 29, 2018

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Jul 29, 2018

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_1109 branch 2 times, most recently from 4716300 to 0e86f4a Jul 30, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

rebased already

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Travis-CI complains:

2393592ms th_a main.cpp:597 test_method ] e.to_detail_string(): 0 exception: unspecified
wallet file cannot be saved
{}
th_a wallet.cpp:824 save_wallet_file
2393594ms p2p thread.cpp:252 exec ] thread canceled: 9 canceled_exception: Canceled
cancellation reason: [none given]
{"reason":"[none given]"}
p2p thread_d.hpp:473 start_next_fiber
2393594ms th_a db_management.cpp:206 close ] Rewinding from 0 to 0
unknown location(0): fatal error in "account_history_pagination": unknown type
/home/travis/build/bitshares/bitshares-core/tests/cli/main.cpp(545): last checkpoint

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Perhaps the Travis-CI environment has different read behavior or limitations? See #1203 (comment) for similar error.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

going to check ... Thanks !

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

I'm playing https://github.com/bitshares/bitshares-core/blob/travis-test1/.travis.yml to check if it's related to disk space.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

wow ! Thanks a lot ! I'm appreciate all your efforts !

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

Not a disk space issue.

$ df -h
Filesystem      Size  Used Avail Use% Mounted on
udev            3.7G  4.0K  3.7G   1% /dev
tmpfs           748M  300K  748M   1% /run
/dev/sda1        30G   18G   11G  62% /
none            4.0K     0  4.0K   0% /sys/fs/cgroup
none            5.0M     0  5.0M   0% /run/lock
none            3.7G     0  3.7G   0% /run/shm
none            100M     0  100M   0% /run/user
none            768M  179M  590M  24% /var/ramfs
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

Question: If saving fails, should the error message contain the JSON representing the wallet file? That might give the user another chance to save the data in an emergency situation, perhaps by screen-scraping it.

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

@pmconrad makes sense.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

#1171 issue concerns about showing any secrets on console, so what is the best approach ? but I also agree with @pmconrad about his idea, it really makes sense and @abitmore was agreed too, let's decide the best approach to be applied everywhere the same way

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Wallet file contains no secret. No password, no plain private key.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

just for the info wallet file contains next info:

{ "chain_id": "4018d7844c78f6a6c41c6a552b898022310fc5dec06da467ee7905a8dad512c8", "my_accounts": [], "cipher_keys": "b30a0ede652bfb2d8d99f8aca77ff8c089f07d77bd047c6df526bc6a652378ed5c37fbbf1d46c408ac1970aec20e83004aee339f1c81e56c0300b00f98f8c142aaad737ddbf7293d339367127a569b1e", "extra_keys": [], "pending_account_registrations": [], "pending_witness_registrations": [], "labeled_keys": [], "blind_receipts": [], "ws_server": "wss://node.bitshares.eu", "ws_user": "", "ws_password": "" }

so no secret is here, just encrypted keys and empty "ws_password" field:

_wallet.cipher_keys = fc::aes_encrypt( data.checksum, plain_txt );

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

but what about "ws_password" field ? isn't it secret ?

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

in my case "ws_password" field is empty so it isn't secret, but it won't always be empty, otherwise we don't need the field that will be always empty

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

I'd say, remove the password before dumping.

_wallet.ws_password = "";
wlog("wallet file ${data}", ("data", fc::json::to_pretty_string( _wallet ) ) );
_wallet.ws_password = ws_password;
if (&_wallet != nullptr)

This comment has been minimized.

Copy link
@pmconrad

pmconrad Aug 2, 2018

Contributor

&_wallet can never be null. It's a struct member of *this.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 2, 2018

Author Member

it fixed travis error with violation memory access in cli_test as was before, strange behavior of travis that's why I've added this check

This comment has been minimized.

Copy link
@pmconrad

pmconrad Aug 2, 2018

Contributor

The travis error is unrelated, please undo this change.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Aug 2, 2018

Author Member

ok sure

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_1109 branch from 909f0bc to d0c7a94 Aug 2, 2018

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

This is still not ready yet. Pushing to next release.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

can we merge this PR and close related issue ? or this is not still ready and need some changes and improvements ? if YES, then which changes and improvements in your opinion ?

Thanks !

Feature release (201810) automation moved this from In progress to In Testing Aug 14, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

Thanks!

@abitmore abitmore merged commit 880a00a into bitshares:develop Aug 17, 2018

1 of 2 checks passed

ci/dockercloud Your tests have been canceled in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Aug 17, 2018

@pmconrad pmconrad referenced this pull request Feb 1, 2019

Open

Merge improvements from BitShares #26

4 of 24 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.