chia-blockchain icon indicating copy to clipboard operation
chia-blockchain copied to clipboard

server: Drop `ChiaServer.validate_broadcast_message_type`

Open xdustinface opened this issue 1 year ago • 8 comments

Purpose:

I might miss the point but this validation there seems strange to me and even more strange that we disconnect/ban from the peers because we tried to initiate sending the "wrong" messages. For me it looks like even sending a message which requires a response doesn't lead to actual issues, does it?

If there is a point in keeping the message_requires_reply checks i might just put it directly into send_to_all but seems to me like just dropping it is fine?

Current Behavior:

The method validates static messages to peers and disconnects from the peers if the code tries to send a message which expects a response.

New Behavior:

It just sends the messages.

xdustinface avatar Sep 01 '23 05:09 xdustinface

Pull Request Test Coverage Report for Build 6045745687

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 32 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.158%

Files with Coverage Reduction New Missed Lines %
chia/wallet/wallet_node.py 2 87.34%
chia/rpc/rpc_server.py 3 87.11%
chia/server/node_discovery.py 3 78.4%
chia/timelord/timelord.py 3 71.9%
chia/wallet/wallet_state_manager.py 4 95.89%
chia/full_node/full_node.py 17 85.79%
<!-- Total: 32
Totals Coverage Status
Change from base Build 6044229157: -0.02%
Covered Lines: 85995
Relevant Lines: 96376

💛 - Coveralls

coveralls-official[bot] avatar Sep 01 '23 07:09 coveralls-official[bot]

did you run into a problem with this check?

arvidn avatar Sep 05 '23 07:09 arvidn

as far as I can tell, this patch removes the check entirely. I agree that it seems reasonable to just drop the message that we attempt to send erroneously, rather than dropping both the peer connection and the message. However, since this would only happen if we have a bug, dropping the peer connection (and the associated co-routines) might make it more likely we chop off the damaged part of the program.

I also would think we really want an error message in the log if this happens.

arvidn avatar Sep 05 '23 07:09 arvidn

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Sep 05 '23 13:09 github-actions[bot]

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

github-actions[bot] avatar Oct 21 '23 11:10 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Dec 11 '23 14:12 github-actions[bot]

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

github-actions[bot] avatar Jan 27 '24 11:01 github-actions[bot]