🚀 fanquake merged a pull request: "doc, test: Document steps to reproduce TSan warning for `libdb`"
(https://github.com/bitcoin/bitcoin/pull/27661)
(https://github.com/bitcoin/bitcoin/pull/27661)
💬 MarcoFalke commented on issue "Drop support for g++-8?":
(https://github.com/bitcoin/bitcoin/issues/27537#issuecomment-1547831477)
Looks like one bug I presumed to be fixed in g++9 is affecting all versions of g++, even 13.1. Maybe someone should report that upstream, because it is valid C++ code and works in clang: https://godbolt.org/z/fM39z38nc
(https://github.com/bitcoin/bitcoin/issues/27537#issuecomment-1547831477)
Looks like one bug I presumed to be fixed in g++9 is affecting all versions of g++, even 13.1. Maybe someone should report that upstream, because it is valid C++ code and works in clang: https://godbolt.org/z/fM39z38nc
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193839090)
Seems good, thanks
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193839090)
Seems good, thanks
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193840138)
Nice, make sense having it in `initialize_setup`, if I have to retouch the code again I can do it.
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193840138)
Nice, make sense having it in `initialize_setup`, if I have to retouch the code again I can do it.
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1547866703)
Force-pushed addressing @MarcoFalke's review.
- Remove cast from `chainstate` - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099662
- Code around `FeeCalculation` has been simplified - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193100074
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1547866703)
Force-pushed addressing @MarcoFalke's review.
- Remove cast from `chainstate` - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099662
- Code around `FeeCalculation` has been simplified - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193100074
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193861398)
Looks like you forgot to address this?
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193861398)
Looks like you forgot to address this?
📝 fanquake opened a pull request: "[23.2] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27663)
Final changes for v23.2. I don't expect any futher backports, or the need to prolong the rc, as the changes here are fairly minimal.
PR for bitcoincore.org is here: https://github.com/bitcoin-core/bitcoincore.org/pull/969
(https://github.com/bitcoin/bitcoin/pull/27663)
Final changes for v23.2. I don't expect any futher backports, or the need to prolong the rc, as the changes here are fairly minimal.
PR for bitcoincore.org is here: https://github.com/bitcoin-core/bitcoincore.org/pull/969
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193863643)
Yea, sorry!
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193863643)
Yea, sorry!
💬 furszy commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1547890992)
Quite sure that this was solved by #26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687)
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1547890992)
Quite sure that this was solved by #26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687)
💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1547892773)
I'm a concept ACK on getting rid of mapRelay. It's been a while since I thought about this and I haven't fully dug through the code again to refresh my memory, but my recollection is that one of the main reasons we had to keep mapRelay in the past was that historically, our software wouldn't deal with NOTFOUND responses to a getdata very well, and so if we announced a transaction and then dropped it (for whatever reason) we would be mildly DoS'ing our peers (I think we used to wait 2 minutes be
...
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1547892773)
I'm a concept ACK on getting rid of mapRelay. It's been a while since I thought about this and I haven't fully dug through the code again to refresh my memory, but my recollection is that one of the main reasons we had to keep mapRelay in the past was that historically, our software wouldn't deal with NOTFOUND responses to a getdata very well, and so if we announced a transaction and then dropped it (for whatever reason) we would be mildly DoS'ing our peers (I think we used to wait 2 minutes be
...
💬 furszy commented on issue "Watch-only, balance not "available" for psbt":
(https://github.com/bitcoin-core/gui/issues/83#issuecomment-1547893224)
Same as in https://github.com/bitcoin/bitcoin/issues/27659. I'm quite sure that this was solved by https://github.com/bitcoin/bitcoin/pull/26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687).
Ping me otherwise and will give it a look.
(https://github.com/bitcoin-core/gui/issues/83#issuecomment-1547893224)
Same as in https://github.com/bitcoin/bitcoin/issues/27659. I'm quite sure that this was solved by https://github.com/bitcoin/bitcoin/pull/26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687).
Ping me otherwise and will give it a look.
💬 fanquake commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1547898820)
> Quite sure that this was solved by https://github.com/bitcoin/bitcoin/pull/26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687)
If that's the case, then this shouldn't be an issue in 25.x, which looks like what is being tested here. Would be great if someone else can confirm/deny.
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1547898820)
> Quite sure that this was solved by https://github.com/bitcoin/bitcoin/pull/26699 (which solved https://github.com/bitcoin/bitcoin/issues/26687)
If that's the case, then this shouldn't be an issue in 25.x, which looks like what is being tested here. Would be great if someone else can confirm/deny.
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1547921439)
Needs rebase
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1547921439)
Needs rebase
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193910577)
Seems the way to do it would be:
```diff
diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp
index c4e1b7d61..fc9b6a5e4 100644
--- a/src/wallet/test/fuzz/fees.cpp
+++ b/src/wallet/test/fuzz/fees.cpp
@@ -15,23 +15,27 @@
namespace wallet {
namespace {
const TestingSetup* g_setup;
+static std::unique_ptr<CWallet> wallet_ptr;
+static Chainstate* chainstate = nullptr;
void initialize_setup()
{
static const auto testing_setup = MakeNoLogFileContext<con
...
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193910577)
Seems the way to do it would be:
```diff
diff --git a/src/wallet/test/fuzz/fees.cpp b/src/wallet/test/fuzz/fees.cpp
index c4e1b7d61..fc9b6a5e4 100644
--- a/src/wallet/test/fuzz/fees.cpp
+++ b/src/wallet/test/fuzz/fees.cpp
@@ -15,23 +15,27 @@
namespace wallet {
namespace {
const TestingSetup* g_setup;
+static std::unique_ptr<CWallet> wallet_ptr;
+static Chainstate* chainstate = nullptr;
void initialize_setup()
{
static const auto testing_setup = MakeNoLogFileContext<con
...
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193914009)
Yeah, lgtm. Maybe use `g_` prefix for the static globals?
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193914009)
Yeah, lgtm. Maybe use `g_` prefix for the static globals?
💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1193919304)
done. liked the idea.
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1193919304)
done. liked the idea.
💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1193920890)
yes! done now.
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1193920890)
yes! done now.
💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193923732)
> Side note: The 50k max inv size also seems to overflow, which is probably another source for false negatives, depending on the use case.
You lost me there. Do you mean this may be a possible source of false negatives for the current path, or for both?
In this patch, I can see that potentially being an issue (that's what the original concern from AJ's comes from), but I don't see how that is an issue for master, given nothing is added to `m_recently_announced_invs` when building this huge
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193923732)
> Side note: The 50k max inv size also seems to overflow, which is probably another source for false negatives, depending on the use case.
You lost me there. Do you mean this may be a possible source of false negatives for the current path, or for both?
In this patch, I can see that potentially being an issue (that's what the original concern from AJ's comes from), but I don't see how that is an issue for master, given nothing is added to `m_recently_announced_invs` when building this huge
...
💬 mzumsande commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1547987039)
> @mzumsande do you want to propose some changes to remove its exposure?
no, I don't want to spend time on discussions on whether some change breaks user space or not. I'd be happy to have an alternative way to query the actual, unfiltered numbers per network, which are much more relevant to _my_ use cases as a developer.
> I don't see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn't include terrible/non-recent (and banned/discou
...
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1547987039)
> @mzumsande do you want to propose some changes to remove its exposure?
no, I don't want to spend time on discussions on whether some change breaks user space or not. I'd be happy to have an alternative way to query the actual, unfiltered numbers per network, which are much more relevant to _my_ use cases as a developer.
> I don't see it as unfortunate; as a frequent user of getnodeaddresses and -addrinfo it seems useful that the data doesn't include terrible/non-recent (and banned/discou
...
💬 MarcoFalke commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193944810)
> You lost me there.
My understanding is that we'd never even send the `inv` at all for anything that exceeds 50k entries, but I might be wrong? If I am not wrong, I wonder if long-term we have no choice other than rate- and size-limit the reply?
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193944810)
> You lost me there.
My understanding is that we'd never even send the `inv` at all for anything that exceeds 50k entries, but I might be wrong? If I am not wrong, I wonder if long-term we have no choice other than rate- and size-limit the reply?