💬 maflcko commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035261425)
It only happens on x86-64, because the asm is in that format.
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035261425)
It only happens on x86-64, because the asm is in that format.
💬 maflcko commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#discussion_r1550236288)
Ah, I see. The page may have only been written for the 18 release, or not included in the 17 release for some reason.
According to commit 4a825039a509c43ba20b2cd7aab448b3be16bcc3, "From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead." C.f. https://web.archive.org/web/20230815180109/https://libcxx.llvm.org/Hardening.html
This makes me wonder if it is worth it to support clang 17 debug mode, if clang 14,15, and 16 isn't supported either.
It seems fine to require clang 18, if som
...
(https://github.com/bitcoin/bitcoin/pull/29781#discussion_r1550236288)
Ah, I see. The page may have only been written for the 18 release, or not included in the 17 release for some reason.
According to commit 4a825039a509c43ba20b2cd7aab448b3be16bcc3, "From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead." C.f. https://web.archive.org/web/20230815180109/https://libcxx.llvm.org/Hardening.html
This makes me wonder if it is worth it to support clang 17 debug mode, if clang 14,15, and 16 isn't supported either.
It seems fine to require clang 18, if som
...
💬 theuni commented on pull request "[WIP] ci: test secp256k1 MSAN asm annotations":
(https://github.com/bitcoin/bitcoin/pull/29742#issuecomment-2035299814)
Ready for bump now that https://github.com/bitcoin-core/secp256k1/pull/1512 is merged.
(https://github.com/bitcoin/bitcoin/pull/29742#issuecomment-2035299814)
Ready for bump now that https://github.com/bitcoin-core/secp256k1/pull/1512 is merged.
💬 achow101 commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035320170)
While this does resolve the compile issue I was having, it does seem to change how gdb is able to debug things, possibly in a meaningful way? This is just an example of a difference that I noticed while stepping through `CWallet::Create` with and without this PR:
On master:
```
Thread 4 "b-httpworker.0" hit Breakpoint 1, wallet::CWallet::Create (context=..., name="funds", database=std::unique_ptr<wallet::WalletDatabase> = {...},
wallet_creation_flags=0, error=..., warnings=std::vect
...
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035320170)
While this does resolve the compile issue I was having, it does seem to change how gdb is able to debug things, possibly in a meaningful way? This is just an example of a difference that I noticed while stepping through `CWallet::Create` with and without this PR:
On master:
```
Thread 4 "b-httpworker.0" hit Breakpoint 1, wallet::CWallet::Create (context=..., name="funds", database=std::unique_ptr<wallet::WalletDatabase> = {...},
wallet_creation_flags=0, error=..., warnings=std::vect
...
🤔 glozow reviewed a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1966413998)
Sorry for the nits, still need to run the fuzzer but otherwise lgtm
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1966413998)
Sorry for the nits, still need to run the fuzzer but otherwise lgtm
💬 glozow commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1550066137)
nit 4f8407cefa6ddb47b166b2c360d03778c0357a00: would prefer to do a restart in the subtest, to minimize the amount of tests that we do on a nondefault settings
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1550066137)
nit 4f8407cefa6ddb47b166b2c360d03778c0357a00: would prefer to do a restart in the subtest, to minimize the amount of tests that we do on a nondefault settings
💬 glozow commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1543093597)
I meant to add docs in the docstring comment :sweat_smile:
also:
- requires `-datacarriersize=100000`
- I think you mean "It will *not* ensure mempools become synced" ?
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1543093597)
I meant to add docs in the docstring comment :sweat_smile:
also:
- requires `-datacarriersize=100000`
- I think you mean "It will *not* ensure mempools become synced" ?
💬 theStack commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035347234)
> I think it would be better if the new format could include a version number so that this could be properly reported.
If we decide to do that, I'd suggest to use the chance and go one step further, by also adding a magic number to the start of the file. This way, it can be quickly identified as being / not being an UTXO dump (for better error reporting, but longer-term also potentially for external identifying tools like [file](https://linux.die.net/man/1/file)). Then we could provide more h
...
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035347234)
> I think it would be better if the new format could include a version number so that this could be properly reported.
If we decide to do that, I'd suggest to use the chance and go one step further, by also adding a magic number to the start of the file. This way, it can be quickly identified as being / not being an UTXO dump (for better error reporting, but longer-term also potentially for external identifying tools like [file](https://linux.die.net/man/1/file)). Then we could provide more h
...
💬 kristapsk commented on pull request "Change example address from legacy (P2PKH) to bech32m (P2TR)":
(https://github.com/bitcoin-core/gui/pull/808#issuecomment-2035390185)
Concept ACK
(https://github.com/bitcoin-core/gui/pull/808#issuecomment-2035390185)
Concept ACK
👍 TheCharlatan approved a pull request: "guix: Remove another leftover from #29648"
(https://github.com/bitcoin/bitcoin/pull/29797#pullrequestreview-1977787840)
ACK 3cb80febb87696f3b1073469c0cc68a57ba81de9
Guix build (x86_64):
```
8e7bddb8fa49c857bc9a815a7f27f2d60f8a2f8955966eff467ee86c0d0776f6 guix-build-3cb80febb876/output/aarch64-linux-gnu/SHA256SUMS.part
de0c0624b55418b6638b4852cbe4527c5fb4a2ccbb82d442e5dff0febc947a70 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3cb80febb876-aarch64-linux-gnu-debug.tar.gz
5f37064b7496125b90f7df1fda7df3e6545149274bf737361579c0e9be94f6f8 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3c
...
(https://github.com/bitcoin/bitcoin/pull/29797#pullrequestreview-1977787840)
ACK 3cb80febb87696f3b1073469c0cc68a57ba81de9
Guix build (x86_64):
```
8e7bddb8fa49c857bc9a815a7f27f2d60f8a2f8955966eff467ee86c0d0776f6 guix-build-3cb80febb876/output/aarch64-linux-gnu/SHA256SUMS.part
de0c0624b55418b6638b4852cbe4527c5fb4a2ccbb82d442e5dff0febc947a70 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3cb80febb876-aarch64-linux-gnu-debug.tar.gz
5f37064b7496125b90f7df1fda7df3e6545149274bf737361579c0e9be94f6f8 guix-build-3cb80febb876/output/aarch64-linux-gnu/bitcoin-3c
...
💬 theuni commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035434442)
@achow101 Yeah, I see the same. Sadly that's the trade-off. Basically `-O0` is a poor approximation of what real optimized code will look like. But anything above is bound to optimize some things out.
So we have to make a choice: pure debug-ability (including things like inlines which don't actually exist), or more realistic binaries while sacrificing some source code in the debugger. Personally I prefer the latter, but you probably live in gdb more than me :)
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035434442)
@achow101 Yeah, I see the same. Sadly that's the trade-off. Basically `-O0` is a poor approximation of what real optimized code will look like. But anything above is bound to optimize some things out.
So we have to make a choice: pure debug-ability (including things like inlines which don't actually exist), or more realistic binaries while sacrificing some source code in the debugger. Personally I prefer the latter, but you probably live in gdb more than me :)
💬 theuni commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035445375)
Note that for local hacking (with clang), you can use:
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 96c4397504..6bc408e843 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2944,7 +2944,7 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
return MakeDatabase(wallet_path, options, status, error_string);
}
-std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::string& name, std::uni
...
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2035445375)
Note that for local hacking (with clang), you can use:
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 96c4397504..6bc408e843 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2944,7 +2944,7 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
return MakeDatabase(wallet_path, options, status, error_string);
}
-std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::string& name, std::uni
...
🤔 murchandamus reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-1977655865)
Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-1977655865)
Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.
💬 murchandamus commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550334355)
Nit: Could use [`std::set::contains`](https://en.cppreference.com/w/cpp/container/set/contains) here and below
```suggestion
BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550334355)
Nit: Could use [`std::set::contains`](https://en.cppreference.com/w/cpp/container/set/contains) here and below
```suggestion
BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
```
💬 murchandamus commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550260907)
In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df):
I am not well-acquainted with `net_processing.cpp`, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for `m_replaced_transactions` in the context of this call?
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550260907)
In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df):
I am not well-acquainted with `net_processing.cpp`, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for `m_replaced_transactions` in the context of this call?
💬 theuni commented on issue "Compilation failure when using `--enable-fuzz` and `--enable-debug` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035485312)
In addition to the discussion in #29781, I'll PR a change to make this work with `DISABLE_OPTIMIZED_SHA256` as a fallback. I'm doing that as part of a larger refactor of the crypto defines, though.
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2035485312)
In addition to the discussion in #29781, I'll PR a change to make this work with `DISABLE_OPTIMIZED_SHA256` as a fallback. I'm doing that as part of a larger refactor of the crypto defines, though.
💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550398421)
@murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it's not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it's an std::optional):
```
The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
static method `MempoolAccepptResult::Success`, which in turn calls the pr
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550398421)
@murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it's not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it's an std::optional):
```
The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
static method `MempoolAccepptResult::Success`, which in turn calls the pr
...
💬 achow101 commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035502334)
> Do you know of any examples of the utxo dumps already being used for something else?
For myself, I have a couple of random scripts that read utxo dumps in order to compute some statistics and other info on the utxo set. I'm not aware of any actual projects using utxo snapshots, but I also haven't actively looked.
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2035502334)
> Do you know of any examples of the utxo dumps already being used for something else?
For myself, I have a couple of random scripts that read utxo dumps in order to compute some statistics and other info on the utxo set. I'm not aware of any actual projects using utxo snapshots, but I also haven't actively looked.
💬 emc99 commented on pull request "guix: remove errant leftover from #29648":
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900)
> ACK [fd8527a](https://github.com/bitcoin/bitcoin/commit/fd8527a20ebc490df030b3a91c1161f00c8a29b6)
>
> Verified the failure, reviewed the change and running a guix build now without any issues (will post the results once it's finished).
When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
(https://github.com/bitcoin/bitcoin/pull/29787#issuecomment-2035514900)
> ACK [fd8527a](https://github.com/bitcoin/bitcoin/commit/fd8527a20ebc490df030b3a91c1161f00c8a29b6)
>
> Verified the failure, reviewed the change and running a guix build now without any issues (will post the results once it's finished).
When you say you are running a guix build, do you mean you are running the guix operating system on a separate machine?
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550445308)
> it is unclear to me from the commit message and the code change how this change fits in the context.
(Note that this commit is a followup from #29619)
Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1550445308)
> it is unclear to me from the commit message and the code change how this change fits in the context.
(Note that this commit is a followup from #29619)
Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.