Skip to content
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

Merged
merged 11 commits into from
Jan 16, 2024

Conversation

yyforyongyu
Copy link
Contributor

@yyforyongyu yyforyongyu commented Nov 4, 2023

This PR expands the RPC client with the new method testmempoolaccept.

  • Missing tests.
  • Catch all possible error messages from testmempoolaccept and map them in btcwallet(or lnd)?

@yyforyongyu yyforyongyu force-pushed the testmempoolaccept branch 2 times, most recently from aac22d3 to b4aca59 Compare November 5, 2023 00:22
@coveralls
Copy link

coveralls commented Nov 5, 2023

Pull Request Test Coverage Report for Build 7526854816

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 56.786%

Totals Coverage Status
Change from base Build 7460741298: 0.002%
Covered Lines: 29208
Relevant Lines: 51435

💛 - Coveralls

@Roasbeef
Copy link
Member

Catch all possible error messages from testmempoolaccept and map them in btcwallet(or lnd)?

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 rpcclient.

Copy link
Member

@Roasbeef Roasbeef left a 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.

btcjson/chainsvrcmds.go Show resolved Hide resolved
btcjson/chainsvrresults.go Show resolved Hide resolved
rpcclient/rawtransactions.go Outdated Show resolved Hide resolved
mempool/mempool.go Show resolved Hide resolved
mempool/policy.go Outdated Show resolved Hide resolved
@yyforyongyu yyforyongyu changed the title WIP: support testmempoolaccept for both bitcoind and btcd support testmempoolaccept for both bitcoind and btcd Dec 5, 2023
@yyforyongyu yyforyongyu force-pushed the testmempoolaccept branch 2 times, most recently from a008686 to 2a23314 Compare December 5, 2023 09:00
@Roasbeef
Copy link
Member

cc @ellemouton

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌻

btcjson/chainsvrresults.go Show resolved Hide resolved
// 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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

rpcserverhelp.go Show resolved Hide resolved
btcutil/go.sum Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member

cc @ellemouton (can't req review as is)

Copy link
Contributor

@ellemouton ellemouton left a 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 🤩

mempool/mempool.go Show resolved Hide resolved
rpcclient/infrastructure_test.go Outdated Show resolved Hide resolved
rpcclient/rawtransactions.go Show resolved Hide resolved
rpcclient/rawtransactions.go Show resolved Hide resolved
rpcclient/rawtransactions.go Show resolved Hide resolved
integration/rawtx_test.go Outdated Show resolved Hide resolved
integration/rawtx_test.go Outdated Show resolved Hide resolved
integration/rawtx_test.go Outdated Show resolved Hide resolved
integration/rawtx_test.go Outdated Show resolved Hide resolved
rpcclient/rawtransactions.go Outdated Show resolved Hide resolved
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`.
@Roasbeef Roasbeef merged commit 17fdc52 into btcsuite:master Jan 16, 2024
3 checks passed
@yyforyongyu yyforyongyu deleted the testmempoolaccept branch January 17, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants