Store a limited quantity of conflicting transactions. This is
currently only used for an RPC but will later be used to vote on these
transactions via avalanche.
Details
Details
- Reviewers
bytesofman PiRK - Group Reviewers
Restricted Project - Commits
- rABC2d31247bc76c: Introduce a conflicting tx pool
./test/functional/test_runner.py rpc_gettransactionstatus
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| doc/release-notes.md | ||
|---|---|---|
| 9 ↗ | (On Diff #49215) | |
| src/net_processing.cpp | ||
| 5800 ↗ | (On Diff #49215) | is this FIXME to be resolved in this diff? |
| src/txconflicting.h | ||
| 12 ↗ | (On Diff #49215) | why 20min? |
| 14 ↗ | (On Diff #49215) | why 5min? |
| test/functional/rpc_gettransactionpool.py | ||
| 46 ↗ | (On Diff #49215) | in general -- the node will reject conflicting txs? this is some kind of regtest override so it is accepted? does this diff change that node behavior? e.g. right now cashtab will not broadcast a tx if "inputs missing or spent", i.e. if a tx is constructed before cashtab has updated its utxo set, so it tries to spend something that it "just spent" ... even if this input is not confirmed -- so will this type of tx broadcast now? |
| test/functional/rpc_gettransactionpool.py | ||
|---|---|---|
| 46 ↗ | (On Diff #49215) | The node still rejects the tx from the mempool, so as of this diff it is still technically rejected and cannot be mined by this node. The send_txs_and_test method attempts to broadcast via the node, which may fail like it does here. It is just kept in a new pool that currently is unused (except for the new RPC) I'm assuming a future diff will make it possible for an avalanche vote to cause the conflicting tx to be transferred to the mempool. So it will not need to be requested again from peers if it turns out to be the final tx. |
| src/net_processing.cpp | ||
|---|---|---|
| 5800 ↗ | (On Diff #49215) | Leftover from the commit split, it is currently added in ProcessInvalidTx() |
| src/txconflicting.h | ||
| 14 ↗ | (On Diff #49215) | There values are copied from the orphan pool and are here to try to free space in the very constrained size of the pool. Under normal circumstances there should be no conflicting tx (just like there should be no orphan) so the size shouldn't matter, but we don't want an attacker to fill the pool indefinitely and make the mechanism fail to work. So if the tx was not removed from the pool (currently only possible if it gets mined/a conflicting tx gets mined; later could be via avalanche) it will expire after a reasonable amount of time. The 20 minutes expiry (checked every 5 minutes) is 2 blocks on average, which is generous enough for a conflicting tx. The 5 minutes interval is there to help doing batch removal rather than removing txs one at a time. |
| test/functional/rpc_gettransactionpool.py | ||
| 46 ↗ | (On Diff #49215) | Pierre has it 100% correct. I think you are confusing orphans and conflicting txs. Orphans are txs for which the node doesn't have the inputs: this might be that a child transaction has been received before the parent. A conflict occurs when a tx is spending the same input as another one, aka a double spend. Under normal circumstances this should never happen, but if it does the network has to reconciliate which one of these conflicting txs is the legit one: the nodes currently apply a "first-seen rule" so the chosen one might differ between nodes depending on which tx they got first. The real "winner" of the conflict is only know when one of the tx is mined. This is the job of pre-consensus to make sure all the nodes select the same tx before it gets mined, and reject the others as well as blocks that would mine a tx that conflicts with the avalanche selected one. |
| src/rpc/rawtransaction.cpp | ||
|---|---|---|
| 1770 ↗ | (On Diff #49226) | Maybe getwhichtransactionpool, when I first saw this name I thought it returned a pool of txs ( like all the mempool txs etc) |
Comment Actions
Another possible name would be gettransactionstatus; you already say this in the release notes: "retrieve the status of a non-mined transaction". It would (kinda) imply that the RPC should return "mined" for mined txs (which we can't do in the general case because the -txindex can be disabled). So either it looks that up (and returns "unknown" if the txindex is off), or the name could also be gettransactionstatusinmemory, which is I think the most precise, albeit a bit long.
Comment Actions
gettransactionstatus was my initial name which I changed just before submitting the diff because it doesn't really reflect the content. But you're the second person to suggest that name together with the mined status (which is really not going to be workable without the txindex as you noticed) so I will roll back. I can add the mined status in another diff.