💬 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?
💬 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-1548009498)
Well, I also coded a test for this there https://github.com/bitcoin/bitcoin/pull/26699/commits/68eed5df8656bed1be6526b014e58d3123102b03.
It seems that the problematic presented here is not that one. Based on the information provided @GregTonoski:
> getwalletinfo
{
"walletname": "test-watch_only",
"walletversion": 169900,
"format": "sqlite",
"balance": 0.00000000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 0.00000000,
"txcount": 0,
"keypoolsize": 0,
"keypoolsize_hd_int
...
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548009498)
Well, I also coded a test for this there https://github.com/bitcoin/bitcoin/pull/26699/commits/68eed5df8656bed1be6526b014e58d3123102b03.
It seems that the problematic presented here is not that one. Based on the information provided @GregTonoski:
> getwalletinfo
{
"walletname": "test-watch_only",
"walletversion": 169900,
"format": "sqlite",
"balance": 0.00000000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 0.00000000,
"txcount": 0,
"keypoolsize": 0,
"keypoolsize_hd_int
...
💬 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_r1193951396)
> You lost me there.
Apologies, I typed something incorrect, again.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193951396)
> You lost me there.
Apologies, I typed something incorrect, again.
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193954353)
Yea, gonna do it!
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193954353)
Yea, gonna do it!
💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548022004)
> I think the last reason to hang on to mapRelay is the reason I gave above
My understanding is that `REPLACED` would also need to be provided to peers: https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255. Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool?
However, that may be solved, but just replacing the `mapRelay` lookup by a `vExtra
...
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548022004)
> I think the last reason to hang on to mapRelay is the reason I gave above
My understanding is that `REPLACED` would also need to be provided to peers: https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255. Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool?
However, that may be solved, but just replacing the `mapRelay` lookup by a `vExtra
...
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548028401)
Force-pushed
- Rebased
- Moved static globals into `initialize_setup` - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099637
Thanks @MarcoFalke for the review
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548028401)
Force-pushed
- Rebased
- Moved static globals into `initialize_setup` - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099637
Thanks @MarcoFalke for the review
💬 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-1548034031)
Still, this is not the case but have to add that a psbt creation on a wallet only with unconfirmed balance isn't possible (coins received from an external source are treated as "untrusted"). In the same way as it is not enabled for regular spending neither in the GUI.
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548034031)
Still, this is not the case but have to add that a psbt creation on a wallet only with unconfirmed balance isn't possible (coins received from an external source are treated as "untrusted"). In the same way as it is not enabled for regular spending neither in the GUI.
💬 kroese commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548035439)
Thanks, when updating the keys I found that `keys.txt` no longer exists in this repo, so I solved it by doing:
```
KEY_URL=https://raw.githubusercontent.com/bitcoin-core/guix.sigs/main/builder-keys
curl -sSL ${KEY_URL}/CoinForensics.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Emzy.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Sjors.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/TheCharlatan.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/achow101.gpg | gpg --import - \
...
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548035439)
Thanks, when updating the keys I found that `keys.txt` no longer exists in this repo, so I solved it by doing:
```
KEY_URL=https://raw.githubusercontent.com/bitcoin-core/guix.sigs/main/builder-keys
curl -sSL ${KEY_URL}/CoinForensics.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Emzy.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Sjors.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/TheCharlatan.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/achow101.gpg | gpg --import - \
...
💬 MarcoFalke commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1548038156)
Should be ready for review
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1548038156)
Should be ready for review
💬 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_r1193975226)
All good, I was just wondering if I may have been missing something.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193975226)
All good, I was just wondering if I may have been missing something.
💬 MarcoFalke commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548044235)
> So why is there not a single file that bundles all the current keys so that I dont have to download them all individually?
No one can know all the current keys, because they can change for every release. However, it is possible to find them for a given release by looking into the folder. For example, you can look into https://github.com/bitcoin-core/guix.sigs/tree/main/25.0rc2 to find all current ones for that tag.
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548044235)
> So why is there not a single file that bundles all the current keys so that I dont have to download them all individually?
No one can know all the current keys, because they can change for every release. However, it is possible to find them for a given release by looking into the folder. For example, you can look into https://github.com/bitcoin-core/guix.sigs/tree/main/25.0rc2 to find all current ones for that tag.
💬 MarcoFalke commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1548047843)
Looks like the only affected code is `constexpr` functions calling `assert(...)`.
Failure can be tested with:
```
git show HEAD~|git apply --reverse # First checkout this pull, then revert the centos 9 bump commit
podman kill ci_i686_centos && MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_i686_centos.sh" ./ci/test_run_all.sh # Run the CI task
```
```
...
qt/addresstablemodel.cpp: In function ‘constexpr AddressTableEntry::Type translateTransactionType(wallet::AddressPurpo
...
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1548047843)
Looks like the only affected code is `constexpr` functions calling `assert(...)`.
Failure can be tested with:
```
git show HEAD~|git apply --reverse # First checkout this pull, then revert the centos 9 bump commit
podman kill ci_i686_centos && MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_i686_centos.sh" ./ci/test_run_all.sh # Run the CI task
```
```
...
qt/addresstablemodel.cpp: In function ‘constexpr AddressTableEntry::Type translateTransactionType(wallet::AddressPurpo
...