🚀 achow101 merged a pull request: "During IBD, prune as much as possible until we get close to where we will eventually keep blocks"
(https://github.com/bitcoin/bitcoin/pull/20827)

👍 kristapsk approved a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1844619925)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513

✅ achow101 closed a pull request: "test: Generate coverage report without running tests"
(https://github.com/bitcoin/bitcoin/pull/28772)

🤔 ryanofsky reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1838949232)
Code review d4ebac8381801d024b69d5f1f5a13246c61edbe4. This seems like a nice change that could make the wallet more reliable, and adds good cleanup and test coverage.

Did not ack just because I noticed a bug in SQLiteBatch::TxnBegin if `m_db` is null, but none of my other comments are too important, so feel free to ignore them.

re: https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093

> In general, we don't have very robust error handling for when the database fails to r
...

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1463377113)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)

It would be good to define -999 as a constant so callers could check for this error. Or if there probably won't be a need for callers to do that, it would be clearer to return SQLITE_ERROR.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466931843)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)

Would suggest calling this `SQliteExecBlocker`, or maybe `SQliteExecHook` or `SQliteTestHook` if you anticipate it being used more broadly for other things in the future. "ExecHandler" seems like the wrong name because the class itself isn't executing anything and doesn't call sqlite3_exec.

Would also suggest adding a comment saying this is used for testing, because otherwise it's not clear w
...

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466947379)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)

It would be nice to have a comment explaining the use of sqlite3_get_autocommit, since the name doesn't really explain what it is doing. Maybe "// sqlite3_get_autocommit returns true by default, and false if a transaction has begun and not been committed or rolled back."

Also, I noticed https://www.sqlite.org/c3ref/get_autocommit.html says "If another thread changes the autocommit status of the dat
...

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466966956)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (508fda2fddfef0cefedde9777c914aec72adf7fb)

I found this comment confusing because I couldn't figure out when failing to abort a transaction would ever be expected to happen, and what the error handling strategy was supposed to be when it did happen. Would maybe suggest: "// If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case by closing and reopening th
...

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974)
In commit "test: sqlite, add coverage for dangling to-be-reverted db txns" (d4ebac8381801d024b69d5f1f5a13246c61edbe4)

I think it would be a little more consistent with other code to call this variable "batch" instead of "handler." Also calling it handler gets a little confusing when code below sets a handler on the handler.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466956009)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)

Would be good to drop the parenthetical about autocommit mode here, since that's now an implementation detail of HasActiveTxn()

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466953925)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)

There appears to be a bug here. Previously it would return false if `m_db` is null. Now it will try to execute begin transaction. I think you need to keep the `!m_database.m_db` condition

👍 theStack approved a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844474487)
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3

Went through another review round and only found some minor nits, mostly naming suggestions. If you retouch (or follow-up material):
* should use multi-line imports (see style guidelines in `test/functional/README.md`)
* commit 382894c3acd2dbf3e4198814f547c75b6fb17706 has a leading space in the commit subject

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466950294)
nit: the name `self.peer` doesn't say a lot, maybe something like `self.key_material` would be more descriptive? (though it's strictly speaking more than that, as it contains also stuff derived from the keys like garbage terminators and session id)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424)
nit: could name this method in a way that it's more obvious that this is called in the asyncio callback context, e.g. `_on_data_v2_handshake`.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466826427)
nit: not a big deal, but I feel this message would fit more to the call-site, when the garbage is actually passed to the transport layer (before `.send_raw_message()`).

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708)
nit: a bit shorter
```suggestion
return self.is_connected and self.last_message.get('version') and not self.supports_v2_p2p
```

📝 achow101 opened a pull request: "consensus: Store transaction nVersion as uin32_t"
(https://github.com/bitcoin/bitcoin/pull/29325)
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.

Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid
...

💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1911072717)
> Yes, I'm analyzing this with the changes related to v3 in mind, in which we will remove the CSV 1.

I don’t know if it’s robust to remove the CSV 1 and potential interactions of invalidated chain of transactions with `m_recent_rejects`.

> But Alice doesn't need the preimage to spend them, this is a revoked commitment, she spends through the revocation path. Unless you're thinking of a different scenario this time?

Still the same scenario than laid out in my comment here: https://githu
...

💬 achow101 commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1911073205)
ACK fa3373d3adbace7e4665cf391363319a55a09a96

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1467012383)
This is gone now.