mempool: Remove time as a key of entries

[rustaceanrob]

The discussion on LimitMempoolSize has indicated that it is possible for a high-fee package to get evicted from the mempool if the parent times out due to the Expire logic. This is a incentive compatibility bug, and it would surprise me if miners have not already removed this behavior.

As a nice side-effect, the mempool benchmarks have gotten faster across the board. Some results from a local benchmark script

                   MEMPOOL BENCHMARK DELTA
==============================================================

  MemPoolAddTransactions
  metric                     before          after          delta        %
  ────────────────────────────────────────────────────────────
  elapsed ms              96.4197       95.1383  -    1.2814     -1.33%
  cpu cycles              347.073M      342.589M -     4.485M    -1.29%
  instructions           1071.378M     1067.754M -     3.623M    -0.34%

  MempoolEviction
  metric                     before          after          delta        %
  ────────────────────────────────────────────────────────────
  elapsed ms               0.0216        0.0211  -    0.0004     -1.91%
  cpu cycles               79.944K       77.821K -     2.123K    -2.66%
  instructions            382.755K      379.597K -     3.157K    -0.82%

  ComplexMemPool
  metric                     before          after          delta        %
  ────────────────────────────────────────────────────────────
  elapsed ms              59.7216       56.7903  -    2.9314     -4.91%
  cpu cycles              225.668M      214.558M -    11.110M    -4.92%
  instructions            507.936M      500.087M -     7.849M    -1.55%

Benchmark script

#!/usr/bin/env python3
"""
Compare Bitcoin Core mempool benchmark performance between two commits.
Builds bench_bitcoin at each commit in isolated git worktrees, runs
MemPoolAddTransactions, MempoolEviction, and ComplexMemPool, then prints
a side-by-side delta table.
Usage:
  ./bench-mempool-delta.py --before <commit> --after <commit> [-j <jobs>]
"""

import argparse
import json
import os
import shutil
import subprocess
import sys
import tempfile
from pathlib import Path

BENCHMARKS = ["MemPoolAddTransactions", "MempoolEviction", "ComplexMemPool"]
FILTER = "|".join(BENCHMARKS)

# (json_key, display_label, scale_to_display, display_unit)
# elapsed is stored in seconds; we display in milliseconds
METRICS = [
  ("median(elapsed)", "elapsed", 1_000, "ms"),
  ("median(cpucycles)", "cpu cycles", 1, ""),
  ("median(instructions)", "instructions", 1, ""),
]

# ANSI colours
GREEN = "\033[32m"
RED = "\033[31m"
YELLOW = "\033[33m"
RESET = "\033[0m"
BOLD = "\033[1m"

USE_COLOUR = sys.stdout.isatty()


def colour(text, code):
  return f"{code}{text}{RESET}" if USE_COLOUR else text


def run(cmd, cwd=None, check=True, capture=False):
  cmd = [str(c) for c in cmd]
  print(f"  $ {' '.join(cmd)}", flush=True)
  return subprocess.run(
      cmd, cwd=str(cwd) if cwd else None, check=check,
      capture_output=capture, text=capture,
  )


def resolve_commit(repo, ref):
  r = subprocess.run(
      ["git", "rev-parse", ref], cwd=str(repo), check=True,
      capture_output=True, text=True,
  )
  return r.stdout.strip()


def short_sha(repo, sha):
  r = subprocess.run(
      ["git", "rev-parse", "--short", sha], cwd=str(repo), check=True,
      capture_output=True, text=True,
  )
  return r.stdout.strip()


def commit_subject(repo, sha):
  r = subprocess.run(
      ["git", "log", "-1", "--format=%s", sha], cwd=str(repo), check=True,
      capture_output=True, text=True,
  )
  return r.stdout.strip()


def detect_cmake_flags(repo):
  """Read compiler choice and wallet setting from the existing build cache."""
  cache = repo / "build" / "CMakeCache.txt"
  flags = {}
  if cache.exists():
      for line in cache.read_text().splitlines():
          if line.startswith("CMAKE_CXX_COMPILER:"):
              flags["CXX"] = line.split("=", 1)[1].strip()
          elif line.startswith("CMAKE_C_COMPILER:"):
              flags["CC"] = line.split("=", 1)[1].strip()
  return flags


def cmake_configure(source, build, cc, cxx, jobs):
  env = os.environ.copy()
  if cc:
      env["CC"] = cc
  if cxx:
      env["CXX"] = cxx

  # Detect whether ninja is available.
  generator = ["-G", "Ninja"] if shutil.which("ninja") else []

  cmd = [
      "cmake", "-B", str(build), "-S", str(source),
      "-DCMAKE_BUILD_TYPE=Release",
      "-DBUILD_BENCH=ON",
      "-DENABLE_WALLET=OFF",   # skip wallet to shorten build
      "-DBUILD_TESTS=OFF",
      "-DBUILD_UTIL=OFF",
      "-DBUILD_DAEMON=OFF",
      "-DBUILD_CLI=OFF",
      "-DBUILD_GUI=OFF",
  ] + generator
  subprocess.run(cmd, check=True, env=env)


def cmake_build(source, build, jobs):
  subprocess.run(
      ["cmake", "--build", str(build), f"-j{jobs}", "--target", "bench_bitcoin"],
      check=True,
  )


def build_at_commit(repo, worktree, build, commit, cc, cxx, jobs):
  print(f"\n{'─'*60}")
  print(f"  Building {commit[:12]}  →  {worktree.name}/")
  print(f"{'─'*60}")
  run(["git", "worktree", "add", str(worktree), commit], cwd=repo)
  cmake_configure(worktree, build, cc, cxx, jobs)
  cmake_build(worktree, build, jobs)


def run_benchmarks(bench_bin, output_json):
  print(f"\n  Running benchmarks…", flush=True)
  subprocess.run(
      [
          str(bench_bin),
          f"-filter={FILTER}",
          f"-output-json={output_json}",
      ],
      check=True,
  )


def load_results(json_path):
  with open(json_path) as f:
      data = json.load(f)
  return {entry["name"]: entry for entry in data["results"]}


def aggregate_runs(run_results):
  """Merge multiple benchmark result dicts by taking the min across runs for each metric.
  Uses min-of-medians: each run's value is already the nanobench median, and
  we take the minimum across runs to reduce OS-scheduling / thermal noise.
  """
  merged = {}
  for bench in BENCHMARKS:
      runs = [r[bench] for r in run_results if bench in r]
      if not runs:
          continue
      entry = dict(runs[0])
      for key, _, _, _ in METRICS:
          vals = [r.get(key, 0) for r in runs]
          entry[key] = min(vals)
      merged[bench] = entry
  return merged


def fmt_val(val, scale):
  v = val * scale
  if abs(v) >= 1_000_000:
      return f"{v/1_000_000:>12.3f}M"
  if abs(v) >= 1_000:
      return f"{v/1_000:>12.3f}K"
  return f"{v:>12.4f} "


def fmt_pct(pct):
  s = f"{pct:+.2f}%"
  if pct < -1:
      return colour(f"{s:>8}", GREEN)
  if pct > 1:
      return colour(f"{s:>8}", RED)
  return colour(f"{s:>8}", YELLOW)


def print_table(before_results, after_results):
  W = 62
  print()
  print(colour(f"{'MEMPOOL BENCHMARK DELTA':^{W}}", BOLD))
  print(colour("=" * W, BOLD))

  for bench in BENCHMARKS:
      br = before_results.get(bench)
      ar = after_results.get(bench)
      if not br or not ar:
          print(f"\n  {bench}: missing data")
          continue

      print(f"\n  {colour(bench, BOLD)}")
      hdr = f"  {'metric':<18} {'before':>14} {'after':>14} {'delta':>14} {'%':>8}"
      print(colour(hdr, BOLD))
      print("  " + "─" * (W - 2))

      for key, label, scale, unit in METRICS:
          b_raw = br.get(key, 0)
          a_raw = ar.get(key, 0)
          b = b_raw * scale
          a = a_raw * scale
          delta = a - b
          pct = (delta / b * 100) if b else 0

          b_s = fmt_val(b_raw, scale)
          a_s = fmt_val(a_raw, scale)
          d_abs = abs(delta)
          sign = "+" if delta >= 0 else "-"
          if d_abs >= 1_000_000:
              d_str = f"{sign}{d_abs/1_000_000:>10.3f}M"
          elif d_abs >= 1_000:
              d_str = f"{sign}{d_abs/1_000:>10.3f}K"
          else:
              d_str = f"{sign}{d_abs:>10.4f} "

          suffix = f" {unit}" if unit else ""
          print(
              f"  {label+suffix:<18} {b_s} {a_s} {d_str}  {fmt_pct(pct)}"
          )

  print()
  print(colour("=" * W, BOLD))
  print(colour("  Green = improvement (lower elapsed/cycles/instructions)", GREEN))
  print(colour("  Red   = regression", RED))


def main():
  parser = argparse.ArgumentParser(
      description="Run mempool benchmarks on two commits and show the delta.",
      formatter_class=argparse.RawDescriptionHelpFormatter,
      epilog=__doc__,
  )
  parser.add_argument("--before", required=True, metavar="COMMIT",
                      help="Baseline commit/branch/tag")
  parser.add_argument("--after", required=True, metavar="COMMIT",
                      help="Candidate commit/branch/tag")
  parser.add_argument("-j", "--jobs", type=int, default=os.cpu_count(),
                      help=f"Parallel build jobs (default: {os.cpu_count()})")
  parser.add_argument("--repo", default=".", metavar="PATH",
                      help="Path to bitcoin repo (default: .)")
  parser.add_argument("--cc", default=None, help="C compiler override")
  parser.add_argument("--cxx", default=None, help="C++ compiler override")
  parser.add_argument("--keep", action="store_true",
                      help="Keep build dirs and JSON results after completion")
  parser.add_argument("--runs", type=int, default=3, metavar="N",
                      help="Number of benchmark runs per commit; results are min-of-medians (default: 3)")
  args = parser.parse_args()

  repo = Path(args.repo).resolve()
  if not (repo / ".git").exists():
      sys.exit(f"error: {repo} is not a git repository")

  # Resolve refs to full SHAs.
  try:
      before_sha = resolve_commit(repo, args.before)
      after_sha = resolve_commit(repo, args.after)
  except subprocess.CalledProcessError as e:
      sys.exit(f"error: could not resolve commit: {e}")

  print(f"\n  before : {short_sha(repo, before_sha)}  {commit_subject(repo, before_sha)}")
  print(f"  after  : {short_sha(repo, after_sha)}  {commit_subject(repo, after_sha)}")
  print(f"  jobs   : {args.jobs}")
  print(f"  runs   : {args.runs} (min-of-medians)")

  # Detect compiler from existing cmake cache unless overridden.
  cmake_flags = detect_cmake_flags(repo)
  cc  = args.cc  or cmake_flags.get("CC")
  cxx = args.cxx or cmake_flags.get("CXX")
  if cc:
      print(f"  CC     : {cc}")
  if cxx:
      print(f"  CXX    : {cxx}")

  tmpdir = Path(tempfile.mkdtemp(prefix="btc-bench-delta-"))
  print(f"  workdir: {tmpdir}")

  try:
      all_results = {}
      for label, sha in [("before", before_sha), ("after", after_sha)]:
          worktree = tmpdir / label
          build    = tmpdir / f"{label}-build"

          build_at_commit(repo, worktree, build, sha, cc, cxx, args.jobs)

          bench_bin = build / "bin" / "bench_bitcoin"
          if not bench_bin.exists():
              sys.exit(f"error: bench binary not found at {bench_bin}")

          run_results = []
          for i in range(args.runs):
              print(f"\n  [{label}] run {i + 1}/{args.runs}", flush=True)
              json_run = tmpdir / f"{label}_run{i}.json"
              run_benchmarks(bench_bin, json_run)
              run_results.append(load_results(json_run))
              if args.keep:
                  print(f"  JSON saved: {json_run}")

          all_results[label] = aggregate_runs(run_results)

      print_table(all_results["before"], all_results["after"])

  except subprocess.CalledProcessError as e:
      sys.exit(f"\nerror: command failed (exit {e.returncode})")
  finally:
      # Always clean up worktrees so git doesn't track stale entries.
      for label in ("before", "after"):
          wt = tmpdir / label
          if wt.exists():
              subprocess.run(
                  ["git", "worktree", "remove", "--force", str(wt)],
                  cwd=str(repo), check=False, capture_output=True,
              )
      if not args.keep:
          shutil.rmtree(tmpdir, ignore_errors=True)
      else:
          print(f"\n  Artifacts kept in: {tmpdir}")


if __name__ == "__main__":
  main()

ref: bitcoin#33510

[ismaelsadeeq]

I mentioned this to you, but will reiterate here; fwiw, I have a slight preference for leaving out the option in startup for a bit, but render it ineffective and marked deprecated in docs for some period of time, and then we do a cleanup later to delete them.

[rustaceanrob]

Resolved in 4dd374c