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

redeclare nChainTx to use uint64_t #29331

Closed
wants to merge 1 commit into from

Conversation

russeree
Copy link
Contributor

Following up on #29258 the type of nChainTx has now been changed to a uint64_t allowing for the counting of TXs well into the future. The note about changing the type in 2024 was also removed. This passes all extended tests.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 26, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29370 (assumeutxo: Get rid of faked nTx and nChainTx values by ryanofsky)
  • #28608 (assumeutxo state and locking cleanup by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@russeree russeree force-pushed the 26-01-24-uint64_t-nchaintx branch 5 times, most recently from 0dce715 to 10d4aa8 Compare January 26, 2024 22:10
@russeree russeree marked this pull request as draft January 26, 2024 22:32
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20917625235

@russeree russeree marked this pull request as ready for review January 27, 2024 16:23
@dergoegge
Copy link
Member

Would you mind adding a test? It should be possible to fake a large nChainTx in a test using assume utxo

@mzumsande
Copy link
Contributor

It should be possible to fake a large nChainTx in a test using assume utxo

It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion here to allow that per RPC could make sense (not necessarily in this PR).

@dergoegge
Copy link
Member

It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion #29261 (comment) to allow that per RPC could make sense (not necessarily in this PR).

I was thinking of a unit test, so no need for an RPC but yes some way to add assumeutxo data for tests would be needed (I assumed we already have this).

@maflcko
Copy link
Member

maflcko commented Feb 5, 2024

The note about changing the type in 2024 was also removed. This passes all extended tests.

I don't think you need to detail what is changed and if the tests pass in the description. It would be better to focus on information that is useful to reviewers. For example, a measurement of memory before and after.

Also, when changing the type, you'll have to check all places where this is used, and update them, if needed. For example, casts should be removed and other types need to be increased in width to hold the new values.

@maflcko
Copy link
Member

maflcko commented Feb 12, 2024

Are you still working on this? If not, I am happy to pick up.

@russeree
Copy link
Contributor Author

Could I please have just a bit more time to work on this?

Thanks, Reese

@fanquake
Copy link
Member

@russeree are you still following up here?

@russeree
Copy link
Contributor Author

@russeree are you still following up here?

I would be willing to hand this off now, primary issue is the inability to allocate the proper amount of time due to other work commitments.

Sorry if this caused any substantial setbacks.

@fanquake
Copy link
Member

No worries. @maflcko may pick it up.

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.

None yet

6 participants