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

add new status and return if tx is orphaned #157

Merged
merged 15 commits into from
Nov 23, 2023
Merged

Conversation

shotasilagadzetaal
Copy link
Collaborator

No description provided.

@@ -17,7 +17,8 @@ enum Status {
SENT_TO_NETWORK = 6;
ACCEPTED_BY_NETWORK = 7;
SEEN_ON_NETWORK = 8;
MINED = 9;
ORPHANED = 9;
MINED = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break the existing tx statuses that are stored, but If we introduce a new status we won't get around it.

As we are already changing the statuses how about using a much higher number for MINED in case we'll have yet more statuses in the future?

e.g. MINED=50

If this is the order of statuses then ORPHANED we maybe should move it before ACCEPTED_BY_NETWORK because an orphaned tx has been sent to network but was not accepted by network. Like ACCEPTED_BY_NETWORK status, the orphaned status is received by zmq.

So I'd put the order in this way.

ORPHANED=7;
ACCEPTED_BY_NETWORK = 8;
SEEN_ON_NETWORK = 9;

MINED=50;

What do you think?

Copy link
Collaborator

@sirdeggen sirdeggen Nov 21, 2023

Choose a reason for hiding this comment

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

Hold on. Let's talk about this. Transactions cannot be orphaned. Only Blocks can be orphaned. No such thing as an orphaned transaction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The prior numeric value is probably completely irrelevant since users don't see this anyway.

Perhaps we should use these numbers in the Response - since it's better than parsing txStatus on the client side, and allows users to set if txStatusCode >= 7 in their codebase for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having said all that - can we define "ORPHANED"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The transaction has been sent to at least 1 Bitcoin node but parent transaction was not found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Impossible situation - why are we broadcasting transactions for which there is no parent transaction?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If one of the UTXOs being spent does not exist then the transaction is REJECTED, surely?

README.md Outdated
| 8 | `ACCEPTED_BY_NETWORK` | The transaction has been accepted by a connected Bitcoin node on the ZMQ interface. If metamorph is not connected to ZMQ, this status will never by set. |
| 9 | `SEEN_ON_NETWORK` | The transaction has been seen on the Bitcoin network and propagated to other nodes. This status is set when metamorph receives an INV message for the transaction from another node than it was sent to. |
| 10 | `SEEN_IN_ORPHAN_MEMPOOL` | The transaction has been sent to at least 1 Bitcoin node but parent transaction was not found. |
| 50 | `MINED` | The transaction has been mined into a block by a mining node. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

These numbers are wrong. Should be like this

  ACCEPTED_BY_NETWORK = 7;
  SEEN_ON_NETWORK = 8;
  MINED = 9;

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@shotasilagadzetaal shotasilagadzetaal merged commit a2dfb90 into main Nov 23, 2023
github-actions bot pushed a commit that referenced this pull request Nov 23, 2023
add new status and return if tx is orphaned
@boecklim boecklim deleted the orphaned-status branch August 28, 2024 11:57
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.

3 participants