send_message is problematic, because it is easy to forget a sync_with_ping (or other wait_until), leading to intermittent test failures. (Example: #31837 (review))
There are more uses of send_and_ping in the codebase than send_message, so in most cases send_and_ping is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming send_message to send_without_ping.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31859.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs |
| Concept ACK | i-am-yuvi |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
(Looks like there are 4 conflicts, so best to wait until there are 0. Otherwise there will be useless churn)
concept ACK, many intermittent test failures end up being this
Concept ACK I agree!
Also, add an explanation for the preference in the docs.
send_message only drops the bytes in a buffer and a sync is needed to
avoid intermittent test issues. Change the name of the method to make
this more apparent during review.
-BEGIN VERIFY SCRIPT-
sed -i 's/send_message(/send_without_ping(/g' $( git grep -l 'send_message(' )
-END VERIFY SCRIPT-rebased and taken out of draft. This should be ready for review, as all conflicts are draft pull requests.
ACK https://github.com/bitcoin/bitcoin/pull/31859/commits/fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d
Some one-time churn that should help future-proof testing code a bit.