💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#issuecomment-1816819522)
re-ACK 4eb2a9ea4b6262bec0bc7c20cb3e684ea75caf42 🖼
<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: re-ACK 4eb2a9ea4b6262bec0bc
...
(https://github.com/bitcoin/bitcoin/pull/28904#issuecomment-1816819522)
re-ACK 4eb2a9ea4b6262bec0bc7c20cb3e684ea75caf42 🖼
<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: re-ACK 4eb2a9ea4b6262bec0bc
...
💬 ajtowns commented on pull request "refactor: Make CTxMemPoolEntry non-copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#discussion_r1397661685)
Replacing a simple copy with listing all the details of the object seems cumbersome. If we want to just prevent accidental copies, how about doing something like this instead:
```c++
class CTxMemPoolEntry {
private:
CTxMemPoolEntry(const CTxMemPoolEntry&) = default;
struct ExplicitCopyTag { explicit ExplicitCopyTag() = default; };
public:
static constexpr ExplicitCopyTag ExplicitCopy;
CTxMemPoolEntry(ExplicitCopyTag /*unused*/, const CTxMemPoolEntry& entry) : CTxMemPool
...
(https://github.com/bitcoin/bitcoin/pull/28903#discussion_r1397661685)
Replacing a simple copy with listing all the details of the object seems cumbersome. If we want to just prevent accidental copies, how about doing something like this instead:
```c++
class CTxMemPoolEntry {
private:
CTxMemPoolEntry(const CTxMemPoolEntry&) = default;
struct ExplicitCopyTag { explicit ExplicitCopyTag() = default; };
public:
static constexpr ExplicitCopyTag ExplicitCopy;
CTxMemPoolEntry(ExplicitCopyTag /*unused*/, const CTxMemPoolEntry& entry) : CTxMemPool
...
💬 hebasto commented on pull request "ci: Avoid toolset ambiguity that MSVC can't handle":
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1816831601)
This PR CI runs:
- Version: 20231029.1.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18791294660
- Version: 20231115.2.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18792841149
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1816831601)
This PR CI runs:
- Version: 20231029.1.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18791294660
- Version: 20231115.2.0: https://github.com/bitcoin/bitcoin/actions/runs/6906384370/job/18792841149
💬 hebasto commented on issue "ci: failure in Windows MSVC build":
(https://github.com/bitcoin/bitcoin/issues/28901#issuecomment-1816833050)
Fixed in https://github.com/bitcoin/bitcoin/pull/28905.
(https://github.com/bitcoin/bitcoin/issues/28901#issuecomment-1816833050)
Fixed in https://github.com/bitcoin/bitcoin/pull/28905.
💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397673476)
Oh, I see you'd add the function as a method to `PeerManagerImpl`. That should be simple enough to push here as well. I'll try to do that next week.
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397673476)
Oh, I see you'd add the function as a method to `PeerManagerImpl`. That should be simple enough to push here as well. I'll try to do that next week.
💬 Sjors commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1816841223)
Note that with cluster mempool #28676 the error would change. Which is another reason why I think it's useful to have tests cases for all four scenario's. But it can wait for another PR, and perhaps @sdaftuar could write it :-)
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1816841223)
Note that with cluster mempool #28676 the error would change. Which is another reason why I think it's useful to have tests cases for all four scenario's. But it can wait for another PR, and perhaps @sdaftuar could write it :-)
💬 ismaelsadeeq commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1816860057)
Thanks @Sjors
I Would also like to work on it whenever that happens.
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1816860057)
Thanks @Sjors
I Would also like to work on it whenever that happens.
💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1816917874)
Force-pushed changed the approach:
- Moved it to `contrib`
- Now it's possible to specify the asmap file and the number of unique ASNs to be used in the test
- When adding an address into new table, check the log: `LogPrint(BCLog::ADDRMAN, "Added %s%s to new[%i][%i]\n"`. It ensures that Core is mapping the addresses correctly according to the ASN.
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1816917874)
Force-pushed changed the approach:
- Moved it to `contrib`
- Now it's possible to specify the asmap file and the number of unique ASNs to be used in the test
- When adding an address into new table, check the log: `LogPrint(BCLog::ADDRMAN, "Added %s%s to new[%i][%i]\n"`. It ensures that Core is mapping the addresses correctly according to the ASN.
💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1816943019)
On November 17, 2023 4:52:53 AM EST, Markus ***@***.***> wrote:
>> The tweet you link shows how pointless this pull-req is.
>
>I want to be in charge of my mempool policy and I want to decide what is spam and what is not, in a straightforward and easy way. This is a matter of principle and principles are not pointless. I don't get why you would want to withhold that from node runners, especially since it is completely opt-in and only people who feel strongly about this will choose to do so.
T
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1816943019)
On November 17, 2023 4:52:53 AM EST, Markus ***@***.***> wrote:
>> The tweet you link shows how pointless this pull-req is.
>
>I want to be in charge of my mempool policy and I want to decide what is spam and what is not, in a straightforward and easy way. This is a matter of principle and principles are not pointless. I don't get why you would want to withhold that from node runners, especially since it is completely opt-in and only people who feel strongly about this will choose to do so.
T
...
💬 TheCharlatan commented on pull request "refactor: Make CTxMemPoolEntry non-copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#discussion_r1397768562)
This looks nice, I'll try to apply it.
(https://github.com/bitcoin/bitcoin/pull/28903#discussion_r1397768562)
This looks nice, I'll try to apply it.
💬 1440000bytes commented on issue "new RPC: sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/issues/28636#issuecomment-1816957857)
> In the meantime, you could presumably do bitcoin-cli sendmsgtopeer 0 "tx" "$txhex"
Tried on regtest. Did not work because I could not see the transaction in peer 0's mempool.
(https://github.com/bitcoin/bitcoin/issues/28636#issuecomment-1816957857)
> In the meantime, you could presumably do bitcoin-cli sendmsgtopeer 0 "tx" "$txhex"
Tried on regtest. Did not work because I could not see the transaction in peer 0's mempool.
⚠️ hebasto opened an issue: "RAM usage regression in 26.x and master on ARM 32-bit"
(https://github.com/bitcoin/bitcoin/issues/28906)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The `bitcoind -dbcache=100 -par=4 -disablewallet -reindex-chainstate` command fails with OOM.
### Expected behaviour
RAM usage does not exceed 0.9 GB.
### Steps to reproduce
100% reproducibility with Guix compiled binaries.
### Relevant log output
```
2023-11-17T18:43:31Z [loadblk] UpdateTip: new best=000000000000000016d35e6ca77dea6f4e2caa387f245a712b1df5bbab3c559c height=314541 ver
...
(https://github.com/bitcoin/bitcoin/issues/28906)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The `bitcoind -dbcache=100 -par=4 -disablewallet -reindex-chainstate` command fails with OOM.
### Expected behaviour
RAM usage does not exceed 0.9 GB.
### Steps to reproduce
100% reproducibility with Guix compiled binaries.
### Relevant log output
```
2023-11-17T18:43:31Z [loadblk] UpdateTip: new best=000000000000000016d35e6ca77dea6f4e2caa387f245a712b1df5bbab3c559c height=314541 ver
...
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816973103)
A regression caused by https://github.com/bitcoin/bitcoin/pull/25325.
cc @martinus @sipa @achow101
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816973103)
A regression caused by https://github.com/bitcoin/bitcoin/pull/25325.
cc @martinus @sipa @achow101
💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1816979340)
> On November 17, 2023 4:52:53 AM EST, Markus ***@***.***> wrote: > The tweet you link shows how pointless this pull-req is. I want to be in charge of my mempool policy and I want to decide what is spam and what is not, in a straightforward and easy way. This is a matter of principle and principles are not pointless. I don't get why you would want to withhold that from node runners, especially since it is completely opt-in and only people who feel strongly about this will choose to do so.
> Thi
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1816979340)
> On November 17, 2023 4:52:53 AM EST, Markus ***@***.***> wrote: > The tweet you link shows how pointless this pull-req is. I want to be in charge of my mempool policy and I want to decide what is spam and what is not, in a straightforward and easy way. This is a matter of principle and principles are not pointless. I don't get why you would want to withhold that from node runners, especially since it is completely opt-in and only people who feel strongly about this will choose to do so.
> Thi
...
💬 hebasto commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1816985984)
> Therefore, I'm in the middle of bisecting using Guix builds.
See: https://github.com/bitcoin/bitcoin/issues/28906.
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1816985984)
> Therefore, I'm in the middle of bisecting using Guix builds.
See: https://github.com/bitcoin/bitcoin/issues/28906.
💬 pinheadmz commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816995291)
Do we have a 32-bit CI task already?
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816995291)
Do we have a 32-bit CI task already?
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816997141)
> Do we have a 32-bit CI task already?
x86, not ARM.
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816997141)
> Do we have a 32-bit CI task already?
x86, not ARM.
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817042102)
This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).
```patch
diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h
index c8e70ebacff..9f93511995e 100644
--- a/src/support/allocators/pool.h
...
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817042102)
This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).
```patch
diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h
index c8e70ebacff..9f93511995e 100644
--- a/src/support/allocators/pool.h
...
💬 ryanofsky commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1397847620)
I think it probably wasn't required to bump libmultiprocess at the same time as bumping capnproto, but good to do it anyway since to provide test coverage for newer code. Changes in the bump were:
* chaincodelabs/libmultiprocess#79
* chaincodelabs/libmultiprocess#83
* chaincodelabs/libmultiprocess#84
* chaincodelabs/libmultiprocess#85
* chaincodelabs/libmultiprocess#86
https://github.com/chaincodelabs/libmultiprocess/compare/1af83d15239ccfa7e47b8764029320953dd7fdf1...61d5a0e661f20a4928
...
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1397847620)
I think it probably wasn't required to bump libmultiprocess at the same time as bumping capnproto, but good to do it anyway since to provide test coverage for newer code. Changes in the bump were:
* chaincodelabs/libmultiprocess#79
* chaincodelabs/libmultiprocess#83
* chaincodelabs/libmultiprocess#84
* chaincodelabs/libmultiprocess#85
* chaincodelabs/libmultiprocess#86
https://github.com/chaincodelabs/libmultiprocess/compare/1af83d15239ccfa7e47b8764029320953dd7fdf1...61d5a0e661f20a4928
...
📝 ryanofsky opened a pull request: "depends: bump libmultiprocess to fix capnproto deprecation warnings"
(https://github.com/bitcoin/bitcoin/pull/28907)
This incorporates PR chaincodelabs/libmultiprocess#88 and reverts the NO_WERROR CI workaround added in #28735
Upstream diff: https://github.com/chaincodelabs/libmultiprocess/compare/61d5a0e661f20a4928fbf868ec9c3c6f17883cc7...414542f81e0997354b45b8ade13ca144a3e35ff1
(https://github.com/bitcoin/bitcoin/pull/28907)
This incorporates PR chaincodelabs/libmultiprocess#88 and reverts the NO_WERROR CI workaround added in #28735
Upstream diff: https://github.com/chaincodelabs/libmultiprocess/compare/61d5a0e661f20a4928fbf868ec9c3c6f17883cc7...414542f81e0997354b45b8ade13ca144a3e35ff1