Skip to content

refactor: Make more transaction size variables signed #28059

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.

Sign up for GitHub

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

Jump to bottom
Merged
glozow merged 1 commit into from
Aug 3, 2023
Merged

refactor: Make more transaction size variables signed #28059

glozow merged 1 commit into from
Aug 3, 2023

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jul 10, 2023 •
edited
Loading

This PR is a continuation of #23962 and it:

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 10, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto hebasto marked this pull request as draft July 10, 2023 06:29
@hebasto hebasto marked this pull request as ready for review July 10, 2023 06:30
@maflcko
Copy link
Member

maflcko commented Jul 10, 2023

Not sure. The other types are 64 bit, so this will overflow eventually

@maflcko
Copy link
Member

maflcko commented Jul 18, 2023

Are you still working on this?

@hebasto hebasto mentioned this pull request Jul 20, 2023
Merged
@hebasto
Copy link
Member Author

hebasto commented Jul 20, 2023 •
edited
Loading

Not sure. The other types are 64 bit, so this will overflow eventually

Are you still working on this?

Since the recent push, the totalSizeWithAncestors is int64_t.

maflcko
maflcko reviewed Jul 20, 2023
View reviewed changes
src/txmempool.cpp Outdated

util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimits(
size_t entry_size,
int32_t entry_size,
Copy link
Member

@maflcko maflcko Jul 20, 2023

Choose a reason for hiding this comment

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

No it is not. this is still int32_t

Copy link
Member Author

@hebasto hebasto Jul 22, 2023

Choose a reason for hiding this comment

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

Thanks! Updated.

@hebasto
refactor: Make more transaction size variables signed
92de74e 
This change gets rid of `static_cast`s and compiler warnings.
@hebasto hebasto changed the title refactor: Make more transaction size variables int32_t refactor: Make more transaction size variables signed Jul 22, 2023
@maflcko
Copy link
Member

maflcko commented Jul 26, 2023

lgtm ACK 92de74e 🥔

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 92de74ef181b42d774bc6b12329bc0c27caf0081  🥔
0ZbMecPPNsqm48/rdeMDLieCAN7U9sumAkW0RxVcIcITZiQOe2nPM1mEeI1s9L8p25tLucM8vrLa+nu2sfmGBg==

@fanquake fanquake requested a review from glozow August 2, 2023 08:39
glozow
glozow reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

ACK 92de74e

@glozow glozow merged commit 7c66a4b into bitcoin:master Aug 3, 2023
@hebasto hebasto deleted the 230710-size branch August 3, 2023 11:55
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
@glozow
Merge bitcoin#28059: refactor: Make more transaction size variables s…
61e007e 
…igned
@bitcoin bitcoin locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@glozow glozow glozow left review comments

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hebasto @DrahtBot @maflcko @glozow