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

Optional fields are required to make bitcoin block, causing errors #381

Closed
MicaiahReid opened this issue Aug 11, 2023 · 6 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@MicaiahReid
Copy link
Contributor

Describe the bug
On this line when standardizing a bitcoin block, the prevout field for a bitcoin transaction is required, and Chainhook throws an error if the field is missing. However, prevout is an optional field, and many transaction inputs don't include it.

This causes many blocks to be skipped and will definitely lead to missed predicate triggers.

To Reproduce
Steps to reproduce the behavior:

  1. Create and run the following predicate, and see that the transaction is not found.
{
  "uuid": "1",
  "name": "Hello Ordinals",
  "chain": "bitcoin",
  "version": 1,
  "networks": {
    "mainnet": {
      "start_block": 801221,
      "end_block": 801221,
      "if_this": {
        "scope": "txid",
        "equals": "0x55c86631e9b08b817e6ebac00b9818d1fd49e0a7d6c00429b6e364d3ef845920"
      },
      "then_that": {
        "file_append": {
          "path": "results/transaction-file-result.json"
        }
      }
    }
  }
}

Expected behavior
A bitcoin block is build event when optional transaction inputs are missing.

@github-project-automation github-project-automation bot moved this to 🆕 New in DevTools Aug 11, 2023
@smcclellan smcclellan added the bug Something isn't working label Aug 14, 2023
@smcclellan smcclellan moved this from 🆕 New to 📋 Backlog in DevTools Aug 14, 2023
@smcclellan smcclellan added this to the Q3-2023 milestone Aug 14, 2023
@qustavo
Copy link
Contributor

qustavo commented Aug 18, 2023

AFAICT prevout is used to compute the fee

sats_in += prevout.value.to_sat();
and
fee: sats_in.saturating_sub(sats_out),
.

I don't see where these are being used apart from just being set:

block_height: prevout.height,
value: prevout.value.to_sat(),

Since fees can be extracted from the block's RPC response, is removing prevout a possible solution here?

@MicaiahReid
Copy link
Contributor Author

We actually found that it was the is_coinbase function that's broken. This PR will fix it.

All transactions except coinbase transactions must have a prevout field, so it's okay to require them.

@qustavo
Copy link
Contributor

qustavo commented Aug 18, 2023

that makes sense!

@smcclellan smcclellan moved this from 📋 Backlog to 👀 In Review in DevTools Aug 21, 2023
@qustavo
Copy link
Contributor

qustavo commented Aug 22, 2023

@MicaiahReid I noticed that the prevout field has been added to bitcoind on version v23.0.0, meaning that previous versions will not have such a field populated. Would it make sense to validate the node's version upon chainhook start and fail if a given version constraint is not met?

@MicaiahReid
Copy link
Contributor Author

@qustavo great find. There's additional discussion here: #391 (comment)

@MicaiahReid
Copy link
Contributor Author

It looks like this occurs when using an outdated version of bitcoind. We will be specifying the requirement to use bitcoind v24+ in #405. Closing this issue as not planned.

@MicaiahReid MicaiahReid closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2023
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in DevTools Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants