BroadcastTransaction is also called by submitpackage RPC.
All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926
It's not maintainable to list all the callers of a function.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, kristapsk, naumenkogs |
| Stale ACK | vasild, BrandonOdiwuor, willcl-ark, shaavan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK cca20d94b787d0881a61509445f4827f913e051d
I was looking at the same comment and was thinking it needs an update. Thanks!
lgtm ACK cca20d94b787d0881a61509445f4827f913e051d
ACK cca20d94b787d0881a61509445f4827f913e051d
ACK cca20d94b787d0881a61509445f4827f913e051d
31 | @@ -32,7 +32,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str 32 | 33 | TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) 34 | { 35 | - // BroadcastTransaction can be called by either sendrawtransaction RPC or the wallet. 36 | + // BroadcastTransaction can be called by sendrawtransaction RPC, submitpackage RPC or the wallet.
I don't think we care about which specific RPCs call it? Would just mention that it can be called by RPC, that's the key info - and more maintainable.
// BroadcastTransaction can be called by RPC or by the wallet.
(this is heading in the direction of "amount of discussion is inversely proportional to the complexity of the change")
When working on another PR, I found this comment useful and I cared with RPC called the function. Surely there are other means to check who is calling a given function, but this comment gives a quick concise answer at a glance. To be useful it has to be precise and elaborate. If it is vague then it is less useful and it might as well be removed completely.
Extending that logic, should we document all the call sites of each helper function that's used by RPCs because it can be helpful sometimes? That seems abnormal to me. I understood the only relevance of this line to be in relation to the next one: "chainman, mempool and peerman are initialized before the RPC server and wallet are started". For this, it doesn't matter which RPCs call it.
BroadcastTransaction() is not just any helper function, it may irreversibly affect user's privacy.
I think BroadcastTransaction can be called by RPC or by the wallet is ambiguous in this case.
By which RPC?
BroadcastTransaction can be invoked by other components that will like to submit transactions to the mempool and relay them to peers, in addition to the wallet and RPC.
It's accessible through the node interface https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/node/interfaces.cpp#L341
If I understand the rationale behind this comment correctly, it's specifically indicating who is currently calling the function, that is sendrawtransaction, submitpackage RPC, and the wallet
But if the rationale is indicating the generic meaning of who can invoke BroadcastTransaction, then yes, I agree it would be even more maintainable to simply delete the comment?
I agree with @stickies-v that listing all the callers of a function within code documentation is not maintainable. This should not be something that a developer relies on and is what a grepper / search bar is for. If it's confusing that not all RPCs are listed, I'd be in favor of just deleting the names of the RPCs that use this function, as it doesn't matter to the comment.
Deleted the comment, and updated the PR description.
You've deleted the comment entirely?
I don't understand why we'd delete it entirely either. It belongs together with the next sentence. I suppose it can be deduced from it too, but the current approach doesn't seem like an improvement to me. I still think my suggested phrasing works best from what I've seen so far. I don't think it's ambiguous, it just doesn't specify which RPC is called, which is irrelevant to the rest of the documentation and as such we shouldn't make it look like it isn't.
Yes I agree with your point. Reverting back with your initial suggestion.
Thanks @stickies-v
ACK 437e8eb219c461d27c73a85e0c57d8e11b72a85d
Following the discussion in the PR so far, I agree that it is more logical and maintainable not to detail which function calls (or can call) a particular function since this information is easily accessible through other methods (e.g. by using an IDE) That's why I think it's best to remove the comment.
BroadcastTransaction is also called by submitpackage RPC.
It's not maintainable to list all the callers of a function.
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea