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

backport: Merge bitcoin#23157, 24041, 20591, 22677 #6259

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

vijaydasmp
Copy link

backporting bitcoin

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157 backport: Merge bitcoin#23157, 19101 Sep 9, 2024
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157, 19101 backport: Merge bitcoin#23157, 24041, 22362, 23307, 23054 Sep 9, 2024
@vijaydasmp vijaydasmp force-pushed the bp23_2024_09_04 branch 4 times, most recently from b9f947a to ebe61aa Compare September 11, 2024 08:05
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157, 24041, 22362, 23307, 23054 backport: Merge bitcoin#23157, 24041, 22362, 23054 Sep 11, 2024
Copy link

This pull request has conflicts, please rebase.

Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157, 24041, 22362, 23054 backport: Merge bitcoin#23157, 24041, 22362, 20591, 23054 Sep 14, 2024
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157, 24041, 22362, 20591, 23054 backport: Merge bitcoin#23157, 24041, 22362, 20591, 22677 Sep 14, 2024
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#23157, 24041, 22362, 20591, 22677 backport: Merge bitcoin#23157, 24041, 20591, 22677 Sep 14, 2024
@vijaydasmp vijaydasmp force-pushed the bp23_2024_09_04 branch 2 times, most recently from 12bae70 to 7f4b4f4 Compare September 15, 2024 08:14
MarcoFalke and others added 4 commits September 15, 2024 14:10
…ce of check() and remove dependency on validation

082c5bf [refactor] pass coinsview and height to check() (glozow)
ed6115f [mempool] simplify some check() logic (glozow)
9e8d7ad [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d1891 MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec [mempool] remove now-unnecessary code (glozow)
54c6f3c [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f [bench] Benchmark CTxMemPool::check() (glozow)
cb14071 [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)

Pull request description:

  Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.

  This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.

ACKs for top commit:
  jnewbery:
    reACK 082c5bf
  GeneFerneau:
    tACK [082c5bf](bitcoin@082c5bf)
  rajarshimaitra:
    tACK bitcoin@082c5bf
  theStack:
    Code-review ACK 082c5bf

Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
b5c9bb5 util: Restore GetIntArg saturating behavior (James O'Beirne)

Pull request description:

  The new locale-independent atoi64 method introduced in bitcoin#20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions.

  Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before bitcoin#20452. Then bitcoin#20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again.

  This change is a stripped-down alternative version of bitcoin#23841 by jamesob

ACKs for top commit:
  jamesob:
    Github ACK bitcoin@b5c9bb5
  vincenzopalazzo:
    ACK bitcoin@b5c9bb5
  MarcoFalke:
    review ACK b5c9bb5 🌘

Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae
…ring rescanning process.

240ea29 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39a test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f1 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)

Pull request description:

  The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
  Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.

  The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
  In the context of rescanning old block, the only time value that as a meaning is the blocktime.

  That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
  This PR Fixes bitcoin#20181.

  To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
  But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (cryptoadvance/specter-desktop#680).

ACKs for top commit:
  jonatack:
    ACK 240ea29 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
  meshcollider:
    re-utACK 240ea29

Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
…ency 2/2

a64078e Break validation <-> txmempool circular dependency (glozow)
64e4963 [mempool] always assert coin spent (glozow)
bb9078e [refactor] put finality and maturity checking into a lambda (glozow)
bedf246 [mempool] only update lockpoints for non-removed entries (glozow)
1b3a11e MOVEONLY: TestLockPointValidity to txmempool (glozow)

Pull request description:

  Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool

  Validation should depend on txmempool (e.g. `CChainstateManager` has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state.

  - Changes `removeForReorg()` to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a `CChainState`. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove.

ACKs for top commit:
  laanwj:
    Code review ACK a64078e
  mjdietzx:
    reACK a64078e
  theStack:
    re-ACK a64078e

Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136
@vijaydasmp
Copy link
Author

Hi @UdjinM6 , This is stuck at ci/gitlab/gitlab.com Expected — Waiting for status to be reported

@UdjinM6
Copy link

UdjinM6 commented Sep 16, 2024

Hi @UdjinM6 , This is stuck at ci/gitlab/gitlab.com Expected — Waiting for status to be reported

Yes, this happens... try rebasing and force-pushing

Copy link

This pull request has conflicts, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants