-
Notifications
You must be signed in to change notification settings - Fork 129
qt: Add option for sending Tx to specific peer in UI #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 29.x-knots
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems strange to have this before the normal sendrawtransaction, but ok :)
Thanks for the review. I will address these comments during the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs proper transaction validation and error handling.
Also from a security perspective there is no permission check (should verify peer accepts the transaction relay) and there is no rate limiting against spamming peers with transactions.
Thanks for the review. Added tx validation and error handling in the last commit. I don't think this feature needs rate limiting. |
1758b48
to
d1a88fd
Compare
d1a88fd
to
9707597
Compare
int total; | ||
std::atomic<bool> hasError{false}; | ||
QString errorMessage; | ||
QStringList peerIds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errorMessage
and peerIds
are accessed from multiple threads without proper synchronization. While there's a mutex for errorMessage
, peerIds
has no protection.
|
||
auto status = std::make_shared<SendStatus>(); | ||
status->total = peerIds.size(); | ||
status->peerIds = peerIds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda captures this (1710-1744) and creates a shared connection that may outlive the RPCConsole
object if the window is closed before all async operations complete.
}); | ||
|
||
for (const QString& peerId : peerIds) { | ||
QString rpcCommand = QString("sendmsgtopeer %1 \"tx\" \"%2\"").arg(peerId).arg(txHex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no validation that sendmsgtopeer
RPC command exists or is available before attempting to use it.
txHex = txHex.replace(QRegularExpression("\\s+"), ""); | ||
|
||
const int MAX_TX_SIZE = 4000000; | ||
if (txHex.size() > MAX_TX_SIZE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MAX_TX_SIZE
check (4MB) is arbitrary and doesn't match protocol limits. Ideally we should use actual protocol constants.
Closes #50