✅ maflcko closed an issue: "Unclear documentation about TX replacements in `gettransaction`"
(https://github.com/bitcoin/bitcoin/issues/27781)

👍 maflcko approved a pull request: "test, assumeutxo: Add test to ensure failure when mempool not empty"
(https://github.com/bitcoin/bitcoin/pull/29394#pullrequestreview-1871091239)
lgtm

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483492912)
please don't remove the trailing comma

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483493614)
why the extra_args juggling? Isn't this set by default already?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1934809271)
Updating my concept ACK. Reading through the numerous comments on this PR, the objections that have been raised have nothing to do with v3, and instead have to do with applications that might use it, and what their best design is. I think applications are free to optimize further, but this PR is not the place to do spec design of layer 2 protocols.

The way I see this issue is that RBF pinning due to the total fee requirement[^1] is a real issue for users, and has been since we first introd
...

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1934821087)
Updated code with all suggestions.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514809)
In 523525707742fc620039cd67e8f1437f7f613a01 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

I think it would be relevant to mention that BDB returns errors on `del()` if it is not in a transaction.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483516605)
In 757d6516db56e2732ea77624e66025446cb816a4 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This could be done in the commit introducing `RunWithinTxn()` rather than changing it in a later commit.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514110)
In 523525707742fc620039cd67e8f1437f7f613a01 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

nit: Could use `RunWithinTxn()`.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483515740)
In 757d6516db56e2732ea77624e66025446cb816a4 "wallet: simplify EraseRecords by using 'ErasePrefix'"

This overload feels unnecessary given that there are only 2 places this function is really used.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514234)
In 523525707742fc620039cd67e8f1437f7f613a01 "wallet: bdb batch 'ErasePrefix', do not create txn internally"

nit: Could use `RunWithinTxn()`.

💬 jonatack commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1934839236)
Concept ACK

💬 mzumsande commented on issue "wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934853492)
> If you can point me in the right direction

I don't really think anyone has actually managed to reproduce the failure (i.e. managed to get the test fail locally with master in the same way it did in the failed CI run, possibly after putting a sleep somewhere). Or if you have managed that, you should describe what exactly you did. Therefore, at this point I don't think anyone knows the right direction, and the main part of solving this issue - which is not necessarily easy - is figuring tha
...

🤔 LarryRuane reviewed a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1871146839)
Thanks for the comments, furszy, force-pushed 7b81dea7eb6b14a939230477835159dad6b4b75b

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1483525174)
It's because after successfully locking the `lockdir` directory, I don't want to release the lock (you had caught that problem earlier), which means we can't delete `.lock`, but I do want to start with a clean data directory. This extra level makes it easy to delete the old data directory (if it exists) without disturbing the `.lock` file so it can remain locked the entire time.

The alternative would be to iterate over the contents of the data directory delete (call `fs::remove_all()`) on eve
...

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1483535959)
It's needed because `fs::remove_all(m_path_root)` removes `m_path_root` (and everything below), so we must re-create `m_path_root` (only the last component of the pathname will need to actually be created). I'll change the `fs::create_directories()` to `TryCreateDirectories()` (even though it shouldn't matter here).

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483541651)
```suggestion
self.restart_node(2 )
```

why is this needed?

💬 theuni commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#discussion_r1483547708)
I think this is a reasonable enough escape hatch. Given that we only have 1 currently, requiring a `// FOO` rather than a `/* FOO */` seems ok to me.

I don't see how we could do it as a clang-tidy plugin unfortunately. Its preprocessor stuff kinda sucks, and I believe we'd also have to keep a hard-coded list because any actual `bitcoin-config.h` would be guaranteed to leave _some_ things undef'd. Hmm, I suppose the plugin itself could read the `.h.in` for each invocation. That's pretty ugly t
...

💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483553999)
Apparently `extra_args ` are needed when calling `restart_node`. Removing it causes an assertion failure later in the code in `run_test` (I also confirmed this with the debugger.)
But you are right that there was some extra juggling in the code with the `*` and `[]` , I fixed it in the latest commit push I've just submitted.

💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1934881847)
> > Include them manually with the exception of some files in crypto:
>
> unrelated question: Is there a reason they need to use the flag at all? The files are never compiled and linked when the flag isn't set, so making them appear "empty" when the flag isn't set is not needed?

Yeah, I agree. If there's some historical reason for this, I've forgotten it. It makes sense to me to make it either optionally compiled or optionally opted in (or out) via a define, but I don't see the need for bo
...