💬 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.
(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.
(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()`.
(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.
(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()`.
(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
(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
...
(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
(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
...
(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).
(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?
(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
...
(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.
(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
...
(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
...
💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483556307)
Please see my answer to your other question.
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483556307)
Please see my answer to your other question.
💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483558372)
There are several headers (serialization comes to mind) that are very handy to use just by copying them out of our tree and using them as-is without a buildsystem. I do this quite often myself. I'd hate to give it up, but it's also become a goal of mine to remove the need for any autoconf defines for low-level files like that. Not a hard goal, just a nice-to-have. And with c++20 we're nearly there. See #29263 for a PR that does a good bit in this regard.
It would also be a nice property for l
...
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483558372)
There are several headers (serialization comes to mind) that are very handy to use just by copying them out of our tree and using them as-is without a buildsystem. I do this quite often myself. I'd hate to give it up, but it's also become a goal of mine to remove the need for any autoconf defines for low-level files like that. Not a hard goal, just a nice-to-have. And with c++20 we're nearly there. See #29263 for a PR that does a good bit in this regard.
It would also be a nice property for l
...
🤔 jonatack reviewed a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1871186180)
Approach ACK modulo the review suggestions.
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1871186180)
Approach ACK modulo the review suggestions.
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1483546926)
In commit bf48c40c0ee62e, I suggest the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 (have updated it to use the best elements of both bf48c40c0ee62e and https://github.com/jonatack/bitcoin/commit/135395c885e72444d9e90b495b101525da827650).
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1483546926)
In commit bf48c40c0ee62e, I suggest the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 (have updated it to use the best elements of both bf48c40c0ee62e and https://github.com/jonatack/bitcoin/commit/135395c885e72444d9e90b495b101525da827650).
💬 araujo88 commented on issue "wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934899985)
> > 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
...
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934899985)
> > 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
...
🤔 jonatack reviewed a pull request: "i2p: log connection was refused due to arbitrary port"
(https://github.com/bitcoin/bitcoin/pull/29393#pullrequestreview-1871242274)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/29393#pullrequestreview-1871242274)
Approach ACK