💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286935217)
this function should be "label aware," meaning if the wallet implements labels later on, this function does not need to change.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286935217)
this function should be "label aware," meaning if the wallet implements labels later on, this function does not need to change.
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286935288)
> The "opposite" type would be any type other than the type itself.
I meant opposite w.r.t `has_witness`, if you have a better name in mind, let me know.
> I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
I'll just add a todo comment about uint256 comparisons.
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286935288)
> The "opposite" type would be any type other than the type itself.
I meant opposite w.r.t `has_witness`, if you have a better name in mind, let me know.
> I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
I'll just add a todo comment about uint256 comparisons.
💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286938038)
Maybe something like this: (no idea if it compiles):
```cpp
template<typename Other>
constexpr int Compare(const Other& other) const {
if contexpr (std::is_same_v<Other, uint256> || \\ TODO remove this
std::is_save_v<Other, transaction_identifier<has_witness>>
){
return reinterpret_cast<const uint256&>(*this).Compare(other);
}else{
static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
}
}
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286938038)
Maybe something like this: (no idea if it compiles):
```cpp
template<typename Other>
constexpr int Compare(const Other& other) const {
if contexpr (std::is_same_v<Other, uint256> || \\ TODO remove this
std::is_save_v<Other, transaction_identifier<has_witness>>
){
return reinterpret_cast<const uint256&>(*this).Compare(other);
}else{
static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
}
}
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941048)
This shouldn't return private keys, it should only return the tweak portion of the private key. This also means `Recipient` doesn't need to have the spend private key.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941048)
This shouldn't return private keys, it should only return the tweak portion of the private key. This also means `Recipient` doesn't need to have the spend private key.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941460)
this should be its own commit
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941460)
this should be its own commit
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941957)
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941957)
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type
📝 Crypt-iQ converted_to_draft a pull request: "p2p: ensure mapBlockSource is removed from in ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.
💬 RicYashiroLee commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669394875)
> Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
>
> The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting th
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669394875)
> Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
>
> The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting th
...
💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669412860)
> Not sure how all of you can be so comfortable with this kind of distortion of status quo, particularly when it serves no one but Peter Todd, some misguided wannabe Core Wizards, and whichever miners Peter can persuade to run code they dont really care about anyway.
FYI excluding Luxor, I haven't had any significant communications with any mining pools. I of course attempted to contact every mining pool I could find contact details for. But other than two I didn't get any responses back, and
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669412860)
> Not sure how all of you can be so comfortable with this kind of distortion of status quo, particularly when it serves no one but Peter Todd, some misguided wannabe Core Wizards, and whichever miners Peter can persuade to run code they dont really care about anyway.
FYI excluding Luxor, I haven't had any significant communications with any mining pools. I of course attempted to contact every mining pool I could find contact details for. But other than two I didn't get any responses back, and
...
👍 MarcoFalke approved a pull request: "crypto: more `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1535733421)
nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3f
...
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1535733421)
nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3f
...
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267199359)
nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace `std::byte{}` with `{}`, if it compiles, same below?
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267199359)
nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace `std::byte{}` with `{}`, if it compiles, same below?
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267206399)
9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:
```cpp
fillrand(MakeWritableByteSpan(ret));
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267206399)
9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:
```cpp
fillrand(MakeWritableByteSpan(ret));
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267215003)
same
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267215003)
same
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267214464)
Any reason to do this? Fuzz inputs may already invalidate for any code change and maintaining compat for some code changes seems inconsistent, will bloat the fuzz code, and cause maintenance overhead in the future.
Would be good to either remove this, or if you *actually* want to maintain compat use a ternary operator:
```cpp
const auto key{fdp.ConsumeBool()?ConsumeFixedLen(...):std::vec<std::byte>(32)};
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267214464)
Any reason to do this? Fuzz inputs may already invalidate for any code change and maintaining compat for some code changes seems inconsistent, will bloat the fuzz code, and cause maintenance overhead in the future.
Would be good to either remove this, or if you *actually* want to maintain compat use a ternary operator:
```cpp
const auto key{fdp.ConsumeBool()?ConsumeFixedLen(...):std::vec<std::byte>(32)};
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286960165)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: SetKey. (no 32)
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286960165)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: SetKey. (no 32)
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286968172)
nit in 2ff930de1c7f4683d0a7a72970683e21bf20ba73: Could use `{}` over `std::byte{}`, if it compiles?
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286968172)
nit in 2ff930de1c7f4683d0a7a72970683e21bf20ba73: Could use `{}` over `std::byte{}`, if it compiles?
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286961781)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: My preference would be to instead base this on a new commit that just does a `input` -> `m_input` ("scripted-diff") rename
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286961781)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: My preference would be to instead base this on a new commit that just does a `input` -> `m_input` ("scripted-diff") rename
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286971030)
unrelated: In C++20, we'll probably have to provide overloads for `operator==(std::span, std::span)`, because equality checks are only defined in the std lib for containers that store memory, it seems.
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286971030)
unrelated: In C++20, we'll probably have to provide overloads for `operator==(std::span, std::span)`, because equality checks are only defined in the std lib for containers that store memory, it seems.
✅ hebasto closed a pull request: "Avoid lock order inversion in `Chainstate::ConnectTip` function"
(https://github.com/bitcoin/bitcoin/pull/27684)
(https://github.com/bitcoin/bitcoin/pull/27684)
💬 TheCharlatan commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669476588)
Re https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668087768
> Can you post a patch that compiles?
Seems like an easy win, no?
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7c74b6b9ff..fe4c3d0f67 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -79,6 +79,8 @@
#include <tuple>
#include <variant>
+#include <boost/signals2/signal.hpp>
+
struct KeyOriginInfo;
using interfaces::FoundBlock;
diff --git a/src/wallet/w
...
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669476588)
Re https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668087768
> Can you post a patch that compiles?
Seems like an easy win, no?
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7c74b6b9ff..fe4c3d0f67 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -79,6 +79,8 @@
#include <tuple>
#include <variant>
+#include <boost/signals2/signal.hpp>
+
struct KeyOriginInfo;
using interfaces::FoundBlock;
diff --git a/src/wallet/w
...