Can you explain why the change from signed to unsigned?

I've updated the commit message. Relevant part used to be:

These int64_t members later
get cast into a size_t in CTxMemPool::CTxMemPool() anyway

Is now:

These limits represent counts and sizes that should never be
negative. Currently, the int64_t members later on in the call stack
get cast into a size_t (without bounds checking) in 
CTxMemPool::CTxMemPool() anyway, so this type change does not
introduce any new underflow risks - it just makes them
unsigned earlier on in the process to minimize back-and-forth
type conversion.

Does that resolve it?

I'm new to the code base so my question may look silly. Why the type of ancestor and descendant count are 64 bit? Isn't 32 bit more than sufficient? Although, I know that's not in the scope of this PR.

Currently, the int64_t members later on in the call stack
get cast into a size_t

Yeah so we have virtual size implemented as size_t, unsigned ints, and signed ints in various places in the codebase, and it would be good to switch to 1 type consistently. I don't think there's a PR to do all of them, but #23962 is one.

My current thinking is to use signed ints instead of size_t because we're doing arithmetic with these limits (see CalculateAncestorsAndCheckLimits, #23962 (comment)). So I always use int64_t to hold virtual sizes. Not sure where we're going long term 🤷, but that's why I don't really agree with switching to unsigned.

Why the type of ancestor and descendant count are 64 bit? Isn't 32 bit more than sufficient?

There's never a need for more than 32 bits, but switching partially would cause integer truncation somewhere.

I understand your reasoning and the one from the linked comment, but I'm not sure it makes sense here. A lot of the comparisons we do are with other size_t's, e.g. from std::set::size(). So, we'd either have to unsafely cast them into an int64_t (probably fine, but... not ideal?) or implement a function that does it with bounds checking etc.

I'm not sure that's preferable? See e.g. the below diff, with some unsafe static_cast as well as implicit conversion (e.g. CheckPackageLimits() passing package.size() to CalculateAncestorsAndCheckLimits())

diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h
index fd0979c6c..584af5679 100644
--- a/src/kernel/mempool_limits.h
+++ b/src/kernel/mempool_limits.h
@@ -17,20 +17,20 @@ namespace kernel {
  */
 struct MemPoolLimits {
     //! The maximum allowed number of transactions in a package including the entry and its ancestors.
-    uint64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT};
+    int64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT};
     //! The maximum allowed size in virtual bytes of an entry and its ancestors within a package.
-    uint64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000};
+    int64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000};
     //! The maximum allowed number of transactions in a package including the entry and its descendants.
-    uint64_t descendant_count{DEFAULT_DESCENDANT_LIMIT};
+    int64_t descendant_count{DEFAULT_DESCENDANT_LIMIT};
     //! The maximum allowed size in virtual bytes of an entry and its descendants within a package.
-    uint64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000};
+    int64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000};
 
     /**
      * @return MemPoolLimits with all the limits set to the maximum
      */
     static MemPoolLimits NoLimits()
     {
-        uint64_t no_limit{std::numeric_limits<uint64_t>::max()};
+        int64_t no_limit{std::numeric_limits<int64_t>::max()};
         return {no_limit, no_limit, no_limit, no_limit};
     }
 
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 3097473ac..ee5ffff7b 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -183,14 +183,14 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256>& vHashes
     }
 }
 
-bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
-                                                  size_t entry_count,
+bool CTxMemPool::CalculateAncestorsAndCheckLimits(int64_t entry_size,
+                                                  int64_t entry_count,
                                                   setEntries& setAncestors,
                                                   CTxMemPoolEntry::Parents& staged_ancestors,
                                                   const Limits& limits,
                                                   std::string &errString) const
 {
-    size_t totalSizeWithAncestors = entry_size;
+    int64_t totalSizeWithAncestors = entry_size;
 
     while (!staged_ancestors.empty()) {
         const CTxMemPoolEntry& stage = staged_ancestors.begin()->get();
@@ -241,7 +241,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
             std::optional<txiter> piter = GetIter(input.prevout.hash);
             if (piter) {
                 staged_ancestors.insert(**piter);
-                if (staged_ancestors.size() + package.size() > limits.ancestor_count) {
+                if (static_cast<int64_t>(staged_ancestors.size() + package.size()) > limits.ancestor_count) {
                     errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
                     return false;
                 }
@@ -277,7 +277,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
             std::optional<txiter> piter = GetIter(tx.vin[i].prevout.hash);
             if (piter) {
                 staged_ancestors.insert(**piter);
-                if (staged_ancestors.size() + 1 > limits.ancestor_count) {
+                if (static_cast<int64_t>(staged_ancestors.size() + 1) > limits.ancestor_count) {
                     errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
                     return false;
                 }

@glozow:

So I always use int64_t to hold virtual sizes. Not sure where we're going long term 🤷, but that's why I don't really agree with switching to unsigned.

I went through all the places where any of the 4 members of MemPoolLimits are accessed. In all of them, except for one (see below), they are immediately cast to a uint. This happens explicitly e.g. here, or implicitly by passing it to a fn/ctor/variable that expects a uint or size_t (e.g. here or here or here).

The only place where we actually use the signedness of any of the members is here:

For that reason, I would argue that updating the MemPoolLimits members to be uint64_t instead of int64_t (which is necessary to avoid compiler warnings that we're comparing uint with int in all of the places outlined the diff here) is - except for the one case - not a regression, and is the most straightforward implementation for the goal of this PR. I currently have no objection to what is being proposed in #23962, but I feel like changing these members and all subsequent call sites to signed integers is orthogonal and would unnecessarily introduce complexity and controversy for this PR. As such, my preference would be to not do it here.

What do you think? If you agree with making MemPoolLimits members uint64_t, I will do a force push that removes the now unneccessary uint casts such as here.

Ah I see, thank you very much for going through the usage! I agree it seems appropriate to defer to a later PR.

3611c2b, approach NACK.

I do not agree that switching to unsigned type is an improvement. Especially for variables involved in arithmetic and comparison operations.

For example, see https://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf

Thanks for your feedback hebasto. Even though the MemPoolLimits members are almost always cast to an unsigned int so I think it'd be clearer to have everything be in the type in which it's actually used, I understand your concern that you don't want to regress the interface to avoid future PRs building on an unsigned MemPoolLimits interface.

As such, I think the most straightforward way to move this PR forward and preserve as much review time as possible is to remove the commit that made the MemPoolLimits members unsigned and introduce static_cast<uint64_t> wherever necessary. This way the interface remains signed and behaviour remains unchanged (including arithmetic with unsigned integers) to how it was before this PR.

I've updated the commit message. Relevant part used to be:

These int64_t members later
get cast into a size_t in CTxMemPool::CTxMemPool() anyway

Is now:

These limits represent counts and sizes that should never be
negative. Currently, the int64_t members later on in the call stack
get cast into a size_t (without bounds checking) in 
CTxMemPool::CTxMemPool() anyway, so this type change does not
introduce any new underflow risks - it just makes them
unsigned earlier on in the process to minimize back-and-forth
type conversion.

Does that resolve it?

I'm new to the code base so my question may look silly. Why the type of ancestor and descendant count are 64 bit? Isn't 32 bit more than sufficient? Although, I know that's not in the scope of this PR.

Ah I see, thank you very much for going through the usage! I agree it seems appropriate to defer to a later PR.

nit:

I beg a pardon for being pedantic :) Feel free to ignore this nit.

Since this is a refactor PR, I'll leave this as is. limitAncestorSize was a uint64_t before, so better to change this in a future PR imo. I'll leave this comment open for visibility.