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

tbcd, bfgd: add various hemi specific indexes #380

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

marcopeereboom
Copy link
Contributor

@marcopeereboom marcopeereboom commented Jan 16, 2025

Summary
Hemi needs to keep track of L2 keystones in order to provide callers the bitcoin view of the hemi network.

Changes
This PR adds L2 keystone indexing and hides this behind a configuration flag.

@marcopeereboom marcopeereboom added type: feature This adds new functionality size: XXL This change is extremely large (+/- 1000+). Changes this large should be split into multiple PRs area: bfg This is a change to BFG (Bitcoin Finality Governor) area: tbc This is a change to TBC (Tiny Bitcoin) labels Jan 16, 2025
@joshuasing joshuasing changed the title tbc/bfg: add various hemi specific indexes tbcd, bfgd: add various hemi specific indexes Jan 16, 2025
@ClaytonNorthey92 ClaytonNorthey92 marked this pull request as draft January 16, 2025 15:48
@@ -1215,20 +1214,387 @@ func (s *Server) TxIndexer(ctx context.Context, endHash *chainhash.Hash) error {
return fmt.Errorf("invalid direction: %v", direction)
}

func processKeystones(blockHash *chainhash.Hash, txs []*btcutil.Tx, kssCache map[chainhash.Hash]tbcd.Keystone) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO before merge: add a test case for this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to a add a test case here, to ensure that we pull the correct bytes off of a btcutil.Tx and store them correctly

@marcopeereboom marcopeereboom marked this pull request as ready for review January 28, 2025 13:40
service/tbc/crawler.go Outdated Show resolved Hide resolved
service/tbc/crawler.go Outdated Show resolved Hide resolved
service/tbc/crawler.go Show resolved Hide resolved
Copy link
Contributor

@ClaytonNorthey92 ClaytonNorthey92 left a comment

Choose a reason for hiding this comment

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

hey @marcopeereboom I have a few changes to request; most requests are around test coverage, there is one indexing question that I have as well.

if err == nil {
t.Fatalf("keystone in db: %v", spew.Sdump(v))
} else {
_, ok := err.(database.NotFoundError)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably better to use errors.Is here

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed👍

name: "invalidRemove",
direction: []int{-1},
kssMap: invalidKssCache,
expectedOutDB: anyKssCache,
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @AL-CT could we define these not as variables, but as each their own map[chainhash.Hash]tbcd.Keystone?

for example:

expectedOutDB: map[chainhash.Hash]tbcd.Keystone{
  ...
}

I fear that sharing variables between tests here may cause unexpected behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

fair, also forced me to make things more readable when fixing it

service/tbc/crawler_test.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: bfg This is a change to BFG (Bitcoin Finality Governor) area: tbc This is a change to TBC (Tiny Bitcoin) size: XXL This change is extremely large (+/- 1000+). Changes this large should be split into multiple PRs type: feature This adds new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants