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

commands: increment address index if current value used for new spend #963

Merged

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Feb 7, 2024

This is a fix to ensure the change index in the database is incremented if a new spend is created with a change address derived from the current index, regardless of whether this new spend is broadcast or not.

EDIT: The same fix has been applied to the receive index.

@@ -261,7 +261,7 @@ impl DaemonControl {
addr_info: &Option<AddrInfo>,
) {
if let Some(AddrInfo { index, is_change }) = addr_info {
if *is_change && db_conn.change_index() < *index {
if *is_change && db_conn.change_index() <= *index {
Copy link
Member

Choose a reason for hiding this comment

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

Should also fix the check against receive index right below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, I'll take a look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now made this fix for the receive index and updated the functional tests.

I didn't test for this in the rust tests as derivation_index_by_address returns None in the dummy DB and so we don't enter the affected function in relation to receive addresses (we do enter the affected function for change addresses that have been generated within create_spend).

@jp1ac4 jp1ac4 changed the title commands: increment change index if current value used for new spend commands: increment address index if current value used for new spend Feb 8, 2024
@darosior darosior self-requested a review February 21, 2024 11:29
@jp1ac4 jp1ac4 force-pushed the commands-increment-index-if-values-equal branch from d562c4e to 323470d Compare March 8, 2024 10:52
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Mar 8, 2024

Rebased on master.

@darosior
Copy link
Member

darosior commented Mar 9, 2024

ACK 323470d -- great catch, thanks.

@darosior darosior force-pushed the commands-increment-index-if-values-equal branch from 323470d to 3a12ea0 Compare March 9, 2024 18:10
@darosior
Copy link
Member

darosior commented Mar 9, 2024

Rebased on master.

@darosior
Copy link
Member

darosior commented Mar 9, 2024

re-ACK bfd6ca5

jp1ac4 added 2 commits March 9, 2024 19:15
This is a fix to ensure the change index in the database is
incremented if a new spend is created with a change
address derived from the current index, regardless of whether
this new spend is broadcast or not.
This is a similar fix as for the change index.
@darosior darosior force-pushed the commands-increment-index-if-values-equal branch from 3a12ea0 to bfd6ca5 Compare March 9, 2024 18:16
@darosior darosior merged commit 147cc0b into wizardsardine:master Mar 9, 2024
18 checks passed
@jp1ac4 jp1ac4 deleted the commands-increment-index-if-values-equal branch March 11, 2024 17:09
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.

2 participants