wallet2: check wallet compatibility with daemon's hard fork version [release] #8544
Conversation
src/cryptonote_basic/hardfork.h
Outdated
| /** | ||
| * @brief returns info for all known hard forks | ||
| */ | ||
| std::vector<hardfork_t> get_hardforks() const { return heights; } |
There was a problem hiding this comment.
It's more efficient to return const std::vector<hardfork_t>& here.
src/cryptonote_core/blockchain.h
Outdated
| * | ||
| * @return the hardforks | ||
| */ | ||
| std::vector<hardfork_t> get_hardforks() const { return m_hardfork->get_hardforks(); } |
There was a problem hiding this comment.
Same, it's more efficient to return const std::vector<hardfork_t>& here.
j-berman
force-pushed
the
wallet-check-hf-version
branch
2 times, most recently
from
c589a34
to
a4d156c
Compare
|
@j-berman looks good. Did you test it on a real v0.17 node? |
|
@SChernykh yep :) here's one: http://support.ablative.hosting:18081 I manually tested every code path on real nodes where possible, and my own modified node where not |
j-berman
force-pushed
the
wallet-check-hf-version
branch
from
a4d156c
to
e292801
Compare
|
Rebased |
| m_rpc_version = resp_t.version; | ||
| for (const auto &hf : resp_t.hard_forks) | ||
| m_daemon_hard_forks.push_back(std::make_pair(hf.hf_version, hf.height)); |
There was a problem hiding this comment.
If this method is called multiple times on the same object, then it's m_daemon_hard_forks field will keep expanding indefinitely.
There was a problem hiding this comment.
Added m_daemon_hard_forks.clear() above the for loop for better defense
j-berman
force-pushed
the
wallet-check-hf-version
branch
from
e292801
to
ab9a27c
Compare
| KV_SERIALIZE(current_height) | ||
| KV_SERIALIZE(target_height) | ||
| KV_SERIALIZE(hard_forks) |
There was a problem hiding this comment.
COMMAND_RPC_GET_VERSION::response_t packets serialized by pre-PR daemons will not be able to be deserialized by post-PR wallets (node_rpc_proxy specifically). I would supply some optional values through KV_SERIALIZE_OPT to maintain backwards compatibility.
Edit: I would also rename entry b/c it's not descriptive (I don't immediately think hard fork height/version pair in the context of getting the version), and other commands have a ::entry member type. Even if it's just something like hf_entry.
There was a problem hiding this comment.
I would supply some optional values through KV_SERIALIZE_OPT
I agree this seems safer, see the latest.
FWIW before this change, I had tested a wallet running the PR's code pointing to pre-PR daemons and vice versa; it worked fine deserializing with 0 values thanks to this. But I prefer handling it more explicitly as you suggested.
j-berman
force-pushed
the
wallet-check-hf-version
branch
from
ab9a27c
to
864a78e
Compare
| { | ||
| if (!silent) | ||
| { | ||
| if (m_wallet->is_offline()) | ||
| fail_msg_writer() << tr("wallet failed to connect to daemon, because it is set to offline mode"); | ||
| else if (wallet_is_outdated) |
There was a problem hiding this comment.
It seems all errors always take this branch, the elseif/else below won't activate. I tried using a v0.18 wallet on a couple v0.17 nodes as well (including the one linked: http://support.ablative.hosting:18081), same thing.
These all return the old wallet error:
monerodonline, but it's v0.17.x.xmonerodonline, but connected to wrong portmonerodoffline, either local or remote- random fake ip:
1.2.3.4:18081 - bad ip:
1:18081(returns wallet outdated error after hanging for a bit)
There was a problem hiding this comment.
Thank you @hinto-janaiyo . Should be fixed in #8585
Overview
Since the fork, there have been a number of support requests caused by pre-fork clients pointing to post-fork daemons and vice versa. This PR ensures hard fork compatibility between client and daemon, and paves the way for a cleaner UX when client and daemon are incompatible. Credit to reddit user u/MoneroArbo for the catalyst to this PR:
Implementation
If an updated wallet tries to connect to a daemon that has not updated for the latest hard fork AND the chain height has passed the fork height, the wallet will fail to connect to that daemon and error out.
If a wallet has not updated for the latest hard fork and tries to connect to an updated daemon AND the chain height has passed the fork height, the wallet will fail to connect to that daemon and error out.
If an updated wallet connects to a daemon that has not updated for the latest fork when the fork height has not yet passed, and then the fork height passes while the wallet is still connected, the wallet will error out when scanning blocks and give a clean error indicating the daemon has not updated.
If a wallet has not updated for the latest fork and connects to an updated daemon when the fork height has not yet passed, and then the fork height passes while the wallet is still connected, the wallet will error out when scanning blocks and give a clean error indicating the wallet has not updated.
Reasoning
I think this versioning approach makes sense over using the RPC versioning system as-is because this approach allows wallets to function smoothly while connecting to incompatible daemons until the hard fork height. It also sets up a nice way for wallets to warn that a hard fork is coming in X blocks/days (though this PR does not give that warning), and to tell users to update either their daemon or wallet as needed, rather than immediately break wallets connecting to incompatible daemons upon release.