👍 laanwj approved a pull request: "doc: archive release notes for v27.2"
(https://github.com/bitcoin/bitcoin/pull/31208#pullrequestreview-2412871966)
ACK 1a05c86ae473d5b615f966203ed1759f2077784a , matches release notes on branch
```
git diff 1a05c86ae473d5b615f966203ed1759f2077784a:doc/release-notes/release-notes-27.2.md 27.x:doc/release-notes.md
```
(https://github.com/bitcoin/bitcoin/pull/31208#pullrequestreview-2412871966)
ACK 1a05c86ae473d5b615f966203ed1759f2077784a , matches release notes on branch
```
git diff 1a05c86ae473d5b615f966203ed1759f2077784a:doc/release-notes/release-notes-27.2.md 27.x:doc/release-notes.md
```
💬 laanwj commented on pull request "doc: Use relative hyperlinks in release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31206#issuecomment-2454521609)
Yes this makes sense, ideally the documentation should work even when it's not on github.
(https://github.com/bitcoin/bitcoin/pull/31206#issuecomment-2454521609)
Yes this makes sense, ideally the documentation should work even when it's not on github.
📝 Abdulkbk opened a pull request: "test: cover edge case for lockunspent rpc"
(https://github.com/bitcoin/bitcoin/pull/31209)
This PR tests the scenario where a negative number is passed as the vout value during the `lockunspent` RPC call. The rationale behind this test is to validate the input parameters for the `lockunspent` function and ensure that the system behaves correctly when given invalid data. By confirming that the error message "Invalid parameter: vout cannot be negative" is returned, we can prevent unexpected behavior or vulnerabilities in the application.
(https://github.com/bitcoin/bitcoin/pull/31209)
This PR tests the scenario where a negative number is passed as the vout value during the `lockunspent` RPC call. The rationale behind this test is to validate the input parameters for the `lockunspent` function and ensure that the system behaves correctly when given invalid data. By confirming that the error message "Invalid parameter: vout cannot be negative" is returned, we can prevent unexpected behavior or vulnerabilities in the application.
💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2454530700)
> maflcko
>
> > They'd have to be cleared on all machines. Let me known if you want that to happen.
>
> I believe this PR is in quite good shape, and it might be worth clearing the depends caches for CI and DrahtBot's Guix builder machines.
Sorry for the delay. I installed DrahtBot from scratch, so that should be clear. The CI may still take a few days. For now, reviewers can ignore the last commit (and just drop it from their review).
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2454530700)
> maflcko
>
> > They'd have to be cleared on all machines. Let me known if you want that to happen.
>
> I believe this PR is in quite good shape, and it might be worth clearing the depends caches for CI and DrahtBot's Guix builder machines.
Sorry for the delay. I installed DrahtBot from scratch, so that should be clear. The CI may still take a few days. For now, reviewers can ignore the last commit (and just drop it from their review).
🤔 maflcko reviewed a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2412892609)
If the build system changes are too cumbersome, maybe for now, only the docs can be updated, so that at least one place reflects the reality that Bitcoin Core on Windows 7 isn't "extensively tested"?
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2412892609)
If the build system changes are too cumbersome, maybe for now, only the docs can be updated, so that at least one place reflects the reality that Bitcoin Core on Windows 7 isn't "extensively tested"?
💬 maflcko commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1827628041)
CI is only testing one of Windows 10, or Windows 11. So I think claiming that both are "extensively" tested may not be true, as limited feedback is available about local testing efforts. (Same for the Linux kernel and macOS).
Maybe just drop "extensively"?
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1827628041)
CI is only testing one of Windows 10, or Windows 11. So I think claiming that both are "extensively" tested may not be true, as limited feedback is available about local testing efforts. (Same for the Linux kernel and macOS).
Maybe just drop "extensively"?
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827628848)
> In [54d81ca](https://github.com/bitcoin/bitcoin/commit/54d81cab53a07fc47ddd38d3be0384651d785558): You're removing this patch, but looking at the upstream source for 6.7.3, it's not immediately clear where this was fixed upstream, and the commit doesn't explain why it's no-longer needed. Can you elaborate?
The patched files are part of the legacy qmake-based build system and are unrelated to the new CMake-based build system.
> I'm wondering if new patches you are introducing are just a re
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827628848)
> In [54d81ca](https://github.com/bitcoin/bitcoin/commit/54d81cab53a07fc47ddd38d3be0384651d785558): You're removing this patch, but looking at the upstream source for 6.7.3, it's not immediately clear where this was fixed upstream, and the commit doesn't explain why it's no-longer needed. Can you elaborate?
The patched files are part of the legacy qmake-based build system and are unrelated to the new CMake-based build system.
> I'm wondering if new patches you are introducing are just a re
...
💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827633761)
Won't we redundantly call `AcceptSubPackage` twice for a single transaction, if `individual_results_nonfinal.emplace(wtxid, single_res);` stuff is triggered?
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827633761)
Won't we redundantly call `AcceptSubPackage` twice for a single transaction, if `individual_results_nonfinal.emplace(wtxid, single_res);` stuff is triggered?
🚀 fanquake merged a pull request: "doc: archive release notes for v27.2"
(https://github.com/bitcoin/bitcoin/pull/31208)
(https://github.com/bitcoin/bitcoin/pull/31208)
⚠️ fanquake opened an issue: "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic"
(https://github.com/bitcoin/bitcoin/issues/31210)
https://cirrus-ci.com/task/5232872305459200:
```bash
067] node0 2024-11-04T11:58:48.064759Z [httpworker.0] [src/wallet/wallet.h:936] [WalletLogPrintf] [default wallet] m_address_book.size() = 2
[06:58:49.067] test 2024-11-04T11:58:48.065000Z TestFramework (INFO): Can upgrade to HD
[06:58:49.067] test 2024-11-04T11:58:48.066000Z TestFramework (ERROR): Assertion failed
[06:58:49.067] Traceback (most recent call last):
[06:58:49.067]
...
(https://github.com/bitcoin/bitcoin/issues/31210)
https://cirrus-ci.com/task/5232872305459200:
```bash
067] node0 2024-11-04T11:58:48.064759Z [httpworker.0] [src/wallet/wallet.h:936] [WalletLogPrintf] [default wallet] m_address_book.size() = 2
[06:58:49.067] test 2024-11-04T11:58:48.065000Z TestFramework (INFO): Can upgrade to HD
[06:58:49.067] test 2024-11-04T11:58:48.066000Z TestFramework (ERROR): Assertion failed
[06:58:49.067] Traceback (most recent call last):
[06:58:49.067]
...
💬 brianacaley commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2454593535)
7
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Gleb Naumenko ***@***.***>
Sent: Monday, November 4, 2024 4:23:27 AM
To: bitcoin/bitcoin ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [bitcoin/bitcoin] p2p: Fill reconciliation sets (Erlay) attempt 2 (PR #30116)
@naumenkogs commented on this pull request.
________________________________
In src/node/txreconciliation.cpp<https://github.com/bitcoin/bitcoin/pull/30116#discussion_r
...
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2454593535)
7
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Gleb Naumenko ***@***.***>
Sent: Monday, November 4, 2024 4:23:27 AM
To: bitcoin/bitcoin ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [bitcoin/bitcoin] p2p: Fill reconciliation sets (Erlay) attempt 2 (PR #30116)
@naumenkogs commented on this pull request.
________________________________
In src/node/txreconciliation.cpp<https://github.com/bitcoin/bitcoin/pull/30116#discussion_r
...
💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic":
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2454599900)
This only reproduces on a specific machine. I'll try to take a closer look.
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2454599900)
This only reproduces on a specific machine. I'll try to take a closer look.
💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454613258)
> `getInt` is used in many RPCs. What makes this call to `getInt` in this RPC special?
Thank you, @maflcko, for your feedback. I believe that `getInt` in `lockunspent` is special because the vout param specifically represents transaction output indexes, which are always whole numbers. Since this value comes directly from user input, providing a descriptive error message would be very helpful.
I also updated the PR description to add motivation for this change.
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454613258)
> `getInt` is used in many RPCs. What makes this call to `getInt` in this RPC special?
Thank you, @maflcko, for your feedback. I believe that `getInt` in `lockunspent` is special because the vout param specifically represents transaction output indexes, which are always whole numbers. Since this value comes directly from user input, providing a descriptive error message would be very helpful.
I also updated the PR description to add motivation for this change.
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827415495)
Nit: Given that `MallocUsage` warns that it `is *not* recursive`, would have been great to add a
```c++
requires std::is_trivial_v<X>
```
here, but apparently it *is* used for nested structures - nothing to do here, just documenting my journey.
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827415495)
Nit: Given that `MallocUsage` warns that it `is *not* recursive`, would have been great to add a
```c++
requires std::is_trivial_v<X>
```
here, but apparently it *is* used for nested structures - nothing to do here, just documenting my journey.
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827342209)
Q: `v.capacity()` can be significantly more here than `v.size()`, right?
Is it because we can't size them properly in https://github.com/bitcoin/bitcoin/blob/master/src/streams.h#L82?
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827342209)
Q: `v.capacity()` can be significantly more here than `v.size()`, right?
Is it because we can't size them properly in https://github.com/bitcoin/bitcoin/blob/master/src/streams.h#L82?
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827424352)
Q: we assume that `sizeof(v)` (the vector's fixed memory) was already included in the calculation before this call, right?
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827424352)
Q: we assume that `sizeof(v)` (the vector's fixed memory) was already included in the calculation before this call, right?
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827341320)
Nit: Could we find better names than X and Y (e.g. to avoid readers thinking Y is the size), maybe something like `template<typename ElementType, typename AllocatorType>`
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827341320)
Nit: Could we find better names than X and Y (e.g. to avoid readers thinking Y is the size), maybe something like `template<typename ElementType, typename AllocatorType>`
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827573203)
Slightly unrelated, but `MemUsage` could be generalized to explain the alignment and rounding:
<details>
<summary>Details</summary>
```C++
static inline size_t MallocUsage(const size_t alloc)
{
if (alloc == 0) return 0;
constexpr size_t alignment{2 * sizeof(void*)};
constexpr size_t round_up{alignment * 2 - 1};
constexpr size_t mask{~(alignment - 1)};
return alloc + round_up & mask;
}
```
and
```C++
BOOST_AUTO_TEST_CASE(malloc_usage)
{
auto referenceM
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827573203)
Slightly unrelated, but `MemUsage` could be generalized to explain the alignment and rounding:
<details>
<summary>Details</summary>
```C++
static inline size_t MallocUsage(const size_t alloc)
{
if (alloc == 0) return 0;
constexpr size_t alignment{2 * sizeof(void*)};
constexpr size_t round_up{alignment * 2 - 1};
constexpr size_t mask{~(alignment - 1)};
return alloc + round_up & mask;
}
```
and
```C++
BOOST_AUTO_TEST_CASE(malloc_usage)
{
auto referenceM
...
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827588117)
I don't actually understand why we're ignoring `m_type`, the comment doesn't help.
Is it because of https://github.com/bitcoin/bitcoin/blob/master/src/netmessagemaker.h#L17, i.e. the string being moved around instead of being owned?
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827588117)
I don't actually understand why we're ignoring `m_type`, the comment doesn't help.
Is it because of https://github.com/bitcoin/bitcoin/blob/master/src/netmessagemaker.h#L17, i.e. the string being moved around instead of being owned?
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827592846)
we still have a few `m_raw_message_size` usages, can you please explain why that's still needed?
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827592846)
we still have a few `m_raw_message_size` usages, can you please explain why that's still needed?