-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
support testmempoolaccept
for both bitcoind
and btcd
#2053
Conversation
aac22d3
to
b4aca59
Compare
Pull Request Test Coverage Report for Build 7526854816
💛 - Coveralls |
I think we can try to map them on the RPC level here? So returning a nice concrete error directly to the caller within the |
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.
First pass of the WIP PR, we should also add an itest for the newly exposed command.
b4aca59
to
bbe772a
Compare
bbe772a
to
d5f1af5
Compare
testmempoolaccept
for both bitcoind
and btcd
testmempoolaccept
for both bitcoind
and btcd
a008686
to
2a23314
Compare
cc @ellemouton |
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.
LGTM 🌻
// NOTE: must be performed after buf.Bytes is copied above. | ||
// | ||
// TODO(yy): remove it once the above TODO is addressed. | ||
if err := tx.Deserialize(buf); err != nil { |
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.
I think I'm missing something here: we just serialized it above, why wouldn't it deserialize here?
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 is because tx.Serialize
doesn't do any check to make sure it's a valid tx, one can pass an empty *wire.MsgTx
to it and it will decode [0 0 0 0 0 0 0 0 0 0]
into the buffer.
2a23314
to
3b15171
Compare
3b15171
to
3845a27
Compare
cc @ellemouton (can't req review as is) |
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.
Very nice 🔥 Ultra clean 🤩
This commit breaks the `maybeAcceptTransaction` into two parts - the first is reading the mempool to validate the transaction, and the relevant logic has been moved into the new method `checkMempoolAcceptance`. The second part is writing to the mempool, and is kept in the method `maybeAcceptTransaction`.
This commit adds bitcoind version 22.0 and 25.0 to our `BackendVersion` set to handle the `testmempoolaccept` RPC calls. A unit test is added to make sure the parser works as expected.
This commit adds a new interface `TxMempool` which defines how other subsystems interact with `TxPool`.
…laccept` This commit creates a `RejectReasonMap` to map the errors returned from `btcd` to bitcoind's `testmempoolaccept` so the `RejectReason` is unified at the RPC level. To make sure the map keys are unique, the error strings are modified in `btcd`.
Also add the `make tidy-module` copied from `lnd`.
3845a27
to
fbe65bf
Compare
This PR expands the RPC client with the new method
testmempoolaccept
.testmempoolaccept
and map them inbtcwallet
(orlnd
)?