Skip to content

Conversation

@panleone
Copy link

@panleone panleone commented Feb 8, 2024

Issue being fixed or feature implemented

Refactor extracted from #5860. Even if that PR should not need anymore this commit (since it has been found a better way to activate dip143), I think It's still a worthy refactor

What was done?

Introduce tx.IsSpecialTxVersion() in place of tx.nVersion == 3, tx.nVersion >= 3
and tx.HasExtraPayloadField() in place of tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL

How Has This Been Tested?

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)


bool IsSpecialTxVersion() const
{
return nVersion >= 3;
Copy link

Choose a reason for hiding this comment

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

NOTE: it's safe to go from == 3 to >=3 because the latter is used in ContextualCheckTransaction

@knst
Copy link
Collaborator

knst commented Feb 9, 2024

wallet_multiwallet.py --usecli | ✖ Failed | 23 s

I tried to re-start CI; everytime this test fails, maybe there's some newly introduced issue? I will try to re-start one more time

The last failure: https://gitlab.com/dashpay/dash/-/jobs/6135684689

@panleone
Copy link
Author

panleone commented Feb 9, 2024

wallet_multiwallet.py --usecli | ✖ Failed | 23 s

I tried to re-start CI; everytime this test fails, maybe there's some newly introduced issue? I will try to re-start one more time

The last failure: https://gitlab.com/dashpay/dash/-/jobs/6135684689

I don't know why it fails, but I have seen it failing in every PR I opened (always in the same job linux64_tsan-test)

@UdjinM6
Copy link

UdjinM6 commented Feb 11, 2024

CI failure is unrelated, should be fixed by #5867

@PastaPastaPasta
Copy link
Member

Hi @panleone; do you intend for this PR to move forward? I would see this likely merged soon with review addressed

@panleone panleone force-pushed the dont_hardcode_tx_version branch 2 times, most recently from 70f4f2c to 5d7fd89 Compare March 4, 2024 20:33
@panleone
Copy link
Author

panleone commented Mar 4, 2024

Hi @panleone; do you intend for this PR to move forward? I would see this likely merged soon with review addressed

Completely forgot about this PR... reviews tackled but I do not have the time to make the history clean so I left 4 extra commits where I applied knst's patches

@UdjinM6 UdjinM6 added this to the 21 milestone Mar 5, 2024
knst
knst previously approved these changes Mar 5, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM, seems as all my suggestions are included

UdjinM6
UdjinM6 previously approved these changes Mar 6, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@github-actions
Copy link

github-actions bot commented Mar 6, 2024

This pull request has conflicts, please rebase.

@PastaPastaPasta
Copy link
Member

Hi @panleone; Sorry, but this PR needs to be rebased, it has conflicts with develop.

@panleone panleone dismissed stale reviews from UdjinM6 and knst via fda6803 March 24, 2024 08:13
@panleone panleone force-pushed the dont_hardcode_tx_version branch from 5d7fd89 to fda6803 Compare March 24, 2024 08:13
@panleone panleone force-pushed the dont_hardcode_tx_version branch from fda6803 to 8b6c96d Compare March 24, 2024 08:21
@panleone
Copy link
Author

Hi @panleone; Sorry, but this PR needs to be rebased, it has conflicts with develop.

should be good now

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 8b6c96d

@knst knst requested review from PastaPastaPasta and UdjinM6 March 24, 2024 18:10
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

@PastaPastaPasta PastaPastaPasta merged commit 938cc70 into dashpay:develop Apr 2, 2024
@UdjinM6 UdjinM6 mentioned this pull request Jul 24, 2024
5 tasks
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.

4 participants