Fix & refactor bootstrap_server
#3861
Conversation
pwojcikdev
added
bug
quality improvements
| if (error) | ||
| { | ||
| status = parse_status::invalid_header; | ||
| callback (boost::system::error_code{}, nullptr); |
There was a problem hiding this comment.
These look like they should be passing in error codes, at the least boost::asio::error::fault
There was a problem hiding this comment.
I thought about passing in error codes, but passing in asio::fault seems wrong as this is not an asio fault. The appropriate thing to do would be to create a nano::error_code::deserialization_error but I didn't think it was worth the effort. Will investigate how to do that.
| } | ||
|
|
||
| void nano::bootstrap_server::run_next (nano::unique_lock<nano::mutex> & lock_a) | ||
| bool nano::bootstrap_server::to_realtime_connection (nano::account const & node_id) |
There was a problem hiding this comment.
To match other code an error code is signalled by returning true, success == 0 == false.
There was a problem hiding this comment.
I found that convention very misleading, seems like a low level C legacy. Let's say we have a check for success using the false for indicating that: 'if !operation_successful())' that reads "if not operation successful" which is logically the opposite of what we want. Needless cognitive overhead in my opinion.
| { | ||
| switch (type) | ||
| { | ||
| case nano::socket::type_t::undefined: |
There was a problem hiding this comment.
For consistency this should be renamed to handshake rather than undefined since it's referenced by is_handshake_connection.
There was a problem hiding this comment.
But calling it handshake would be incorrect for bootstrap connections. Wouldn't it?
We could possibly have an additional state calling handshake when we have started a handshake but before completing it.
There was a problem hiding this comment.
I think is_handshake_connection() should be renamed to is_undefined_connection() because that is exactly what it is checking.
There was a problem hiding this comment.
Could be renamed, it's true that there won't be any handshake if the connection is used for bootstrapping. I would move the whole socket type tracking into bootstrap_server (and rename that to network_server) because at the end of the day socket should be oblivious as to what it's used for, this is the task for higher layers
pwojcikdev
force-pushed
the
bootstrap-server-fix-1
branch
6 times, most recently
from
49296cb
to
1a21512
Compare
pwojcikdev
force-pushed
the
bootstrap-server-fix-1
branch
from
d52131e
to
7f16250
Compare
No description provided.