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

NoteSource changes broke notes_by_sender and hence the summoner #3568

Closed
hdevalence opened this issue Jan 3, 2024 · 4 comments · Fixed by #3604
Closed

NoteSource changes broke notes_by_sender and hence the summoner #3568

hdevalence opened this issue Jan 3, 2024 · 4 comments · Fixed by #3604

Comments

@hdevalence
Copy link
Member

Describe the bug

In #3527 I killed the NoteSource "stuffed" transaction hash approach in favor of an enum. However, this had an unintended effect of breaking notes_by_sender:

            JOIN tx ON spendable_notes.source = tx.tx_hash

Because the source column in SQLite is the proto encoding, it no longer matches up with the hash column of the tx table.

Presumably this is why the summoner smoke test has been failing.

@conorsch
Copy link
Contributor

conorsch commented Jan 3, 2024

Presumably we could have caught this by running the summoner smoke tests on PRs. While I don't support a philosophy of "run every single test on every PR," given that we're already running the smoke tests on every PR, which take 17m, and the summoner smokes only take ~11m, when last passing, we wouldn't be losing any wall time. So I'm inclined to add them, as long as they run in parallel.

Objections?

@hdevalence
Copy link
Member Author

I think the system worked in this case, I saw the error and chose to ignore it until now, and now we have an issue to fix it.

@hdevalence
Copy link
Member Author

hdevalence commented Jan 4, 2024

Clue on how we can fix this: we just need to account for a 4-byte 0a220a20 prefix indicating a transaction hash commitment source

source1_hex: 0a220a200101010101010101010101010101010101010101010101010101010101010101
source2_hex: 0a220a200202020202020202020202020202020202020202020202020202020202020202

@hdevalence
Copy link
Member Author

hdevalence commented Jan 4, 2024

#3569 has an ugly fix for this. I'm wondering if a better fix would be to add an extra tx_hash field to the notes table that's either NULL (in case there is no corresponding transaction) or the tx hash directly (if the CommitmentSource is CommitmentSource::Transaction { ... }). Having to do SUBSTR matching on a fixed-length prefix that happens to be the proto bytes is ... not ideal (cc @aubrika)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Status: Testnet 65: Deimos
Development

Successfully merging a pull request may close this issue.

2 participants