🤔 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
💬 jonatack commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483580775)
Suggest making this log output more helpful like the one it replaces by providing the addr, and also making it more similar to the other I2P error logs.
```suggestion
Log("Error connecting to %s, connection refused due to arbitrary port %s", to.ToStringAddrPort(), to.GetPort());
```
<details><summary>full diff with test</summary><p>
```diff
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -217,7 +217,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error)
...
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483580775)
Suggest making this log output more helpful like the one it replaces by providing the addr, and also making it more similar to the other I2P error logs.
```suggestion
Log("Error connecting to %s, connection refused due to arbitrary port %s", to.ToStringAddrPort(), to.GetPort());
```
<details><summary>full diff with test</summary><p>
```diff
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -217,7 +217,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error)
...
💬 hernanmarino commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483584784)
By debugging and digging deeper in the code, I see that the extra_args are preserved only if you call setup_nodes() or if you call add_nodes() with the extra_args parameter, something that was not done here by the original author (intentionally, I guess)
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483584784)
By debugging and digging deeper in the code, I see that the extra_args are preserved only if you call setup_nodes() or if you call add_nodes() with the extra_args parameter, something that was not done here by the original author (intentionally, I guess)
👍 ryanofsky approved a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1871254500)
Light code review ACK 757d6516db56e2732ea77624e66025446cb816a4, nice simplification of EraseRecords
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1871254500)
Light code review ACK 757d6516db56e2732ea77624e66025446cb816a4, nice simplification of EraseRecords
💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483588570)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (a8a24cb20974cb8eaec8cb438f6b22489719d605)
Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483588570)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (a8a24cb20974cb8eaec8cb438f6b22489719d605)
Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object
💬 jonatack commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934956278)
My usual fuzz configure for ARM64 macOS is the one in `doc/fuzzing.md`.
Just tried building with and without the `--disable-asm` option and then running `FUZZ=random ./src/test/fuzz/fuzz`.
The one difference I notice without `--disable-asm` is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):
```
crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x62d000000406 for type 'uint64_t' (aka 'unsigned long long'), wh
...
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934956278)
My usual fuzz configure for ARM64 macOS is the one in `doc/fuzzing.md`.
Just tried building with and without the `--disable-asm` option and then running `FUZZ=random ./src/test/fuzz/fuzz`.
The one difference I notice without `--disable-asm` is this additional entry near the beginning of the fuzz run (after which the fuzzer continues to run):
```
crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x62d000000406 for type 'uint64_t' (aka 'unsigned long long'), wh
...
💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483608953)
There is an extra space after the extra_args option.
Since the `-persistmempool=0` is only relevant to this one restart and not the rest of the tests I wouldn't change the setup and instead do it like this, but how it's done now is also ok.
Another small improvement might be to switch from node 2 to node 0. None of the args from node 2 are needed for this test, so using node 0, that has no args, may cause less confusion.
```suggestion
# Restart to purge the transaction from th
...
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483608953)
There is an extra space after the extra_args option.
Since the `-persistmempool=0` is only relevant to this one restart and not the rest of the tests I wouldn't change the setup and instead do it like this, but how it's done now is also ok.
Another small improvement might be to switch from node 2 to node 0. None of the args from node 2 are needed for this test, so using node 0, that has no args, may cause less confusion.
```suggestion
# Restart to purge the transaction from th
...
💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483610162)
Good suggestion.
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483610162)
Good suggestion.