💬 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.
💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483610267)
Actually, when you switch to use node 0 you can remove this line entirely because node 0 is not used to load snapshots in the rest of the test.
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483610267)
Actually, when you switch to use node 0 you can remove this line entirely because node 0 is not used to load snapshots in the rest of the test.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483614824)
Hmmm I find early returns in the middle of a function a bit brittle. Would be easier to write a bug if e.g. we added another v3 rule at the bottom of the function.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483614824)
Hmmm I find early returns in the middle of a function a bit brittle. Would be easier to write a bug if e.g. we added another v3 rule at the bottom of the function.
💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483614887)
@hernanmarino you marked the other thread as resolved while I was writing, this change works and I think that's what @maflcko had in mind: https://github.com/fjahr/bitcoin/commit/a6883b8919d7348bab56f6ebee0818912146ae52
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483614887)
@hernanmarino you marked the other thread as resolved while I was writing, this change works and I think that's what @maflcko had in mind: https://github.com/fjahr/bitcoin/commit/a6883b8919d7348bab56f6ebee0818912146ae52
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483617248)
fair enough, I just keep forgetting I can assume it has a parent because this block is pretty large. maybe a skill issue on my part
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483617248)
fair enough, I just keep forgetting I can assume it has a parent because this block is pretty large. maybe a skill issue on my part
💬 dergoegge commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934972190)
@jonatack see https://github.com/bitcoin/bitcoin/issues/29178 (we prefer having this broken over reverting the change that introduced it for some reason)
I'm not sure if the `random` harness would trigger the ASan error, the docs mention sha256 which `random` does not use afaict. Could you try `txorphan` or a different harness that does some hashing?
Did you set `UBSAN_OPTIONS` to include `halt_on_error=1`? that is likely why it keeps running if you didn't
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934972190)
@jonatack see https://github.com/bitcoin/bitcoin/issues/29178 (we prefer having this broken over reverting the change that introduced it for some reason)
I'm not sure if the `random` harness would trigger the ASan error, the docs mention sha256 which `random` does not use afaict. Could you try `txorphan` or a different harness that does some hashing?
Did you set `UBSAN_OPTIONS` to include `halt_on_error=1`? that is likely why it keeps running if you didn't
💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1934972685)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483580775
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1934972685)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1483580775