🚀 ryanofsky merged a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115)
(https://github.com/bitcoin/bitcoin/pull/30115)
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611850253)
Same here. I'll take them out
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611850253)
Same here. I'll take them out
💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611850452)
old habits!
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611850452)
old habits!
💬 theuni commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611851597)
Why is this needed?
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611851597)
Why is this needed?
💬 ryanofsky commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127356721)
Went ahead and merged this. It seems the increased peak memory usage is not a major concern, and other metrics like number of allocations and run time do seem improved.
Marco seems right that naively calling `shrink_to_fit might` not help much, or could be basically equivalent to copying, and there might be better ways to decrease memory usage, such as with a `write_and_destroy` method.
(Note: I also edited PR description to remove @ from <span>@</span>willcl-ark, so Will doesn't receive g
...
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127356721)
Went ahead and merged this. It seems the increased peak memory usage is not a major concern, and other metrics like number of allocations and run time do seem improved.
Marco seems right that naively calling `shrink_to_fit might` not help much, or could be basically equivalent to copying, and there might be better ways to decrease memory usage, such as with a `write_and_destroy` method.
(Note: I also edited PR description to remove @ from <span>@</span>willcl-ark, so Will doesn't receive g
...
💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127371384)
@ryanofsky Thanks.
I'm going to continue working on the more complicated copies in follow-up PRs. I'll play around with your `Copy`/`CopyFrom` idea while I'm at it. If nothing else, that would be a good way to help me track down the remaining cases.
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127371384)
@ryanofsky Thanks.
I'm going to continue working on the more complicated copies in follow-up PRs. I'll play around with your `Copy`/`CopyFrom` idea while I'm at it. If nothing else, that would be a good way to help me track down the remaining cases.
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611867224)
> Would also be nice if we prevented these objects from be constructed with `nullptrs`. I'm not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.
It looks like I may be able to do this by deleting the constructor
```
// Deleted constructor
ReplacedReason(std::nullptr_t) = delete;
```
I'll test it
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611867224)
> Would also be nice if we prevented these objects from be constructed with `nullptrs`. I'm not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.
It looks like I may be able to do this by deleting the constructor
```
// Deleted constructor
ReplacedReason(std::nullptr_t) = delete;
```
I'll test it
💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611867777)
Oh right, I missed that. That mitigates my main concern, and given that this hack is going away eventually, I agree on the lack of urgency.
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611867777)
Oh right, I missed that. That mitigates my main concern, and given that this hack is going away eventually, I agree on the lack of urgency.
💬 achow101 commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-2127373603)
There seems to be an intermittent failure with `feature_discover.py`. 2 of 4 functional test suite runs that I just did failed there with:
```
175/307 - feature_discover.py failed, Duration: 1 s
stdout:
2024-05-23T15:01:57.868000Z TestFramework (INFO): PRNG seed is: 6285884694359884871
2024-05-23T15:01:57.869000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240523_110117/feature_discover_78
2024-05-23T15:01:58.127000Z TestFramework (INFO): Restart node with
...
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-2127373603)
There seems to be an intermittent failure with `feature_discover.py`. 2 of 4 functional test suite runs that I just did failed there with:
```
175/307 - feature_discover.py failed, Duration: 1 s
stdout:
2024-05-23T15:01:57.868000Z TestFramework (INFO): PRNG seed is: 6285884694359884871
2024-05-23T15:01:57.869000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240523_110117/feature_discover_78
2024-05-23T15:01:58.127000Z TestFramework (INFO): Restart node with
...
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611882820)
> Also they're variable-length
That makes sense. Whereas `rt_msghdr` is fixed length I guess, so you're able to use `rt + 1` above.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611882820)
> Also they're variable-length
That makes sense. Whereas `rt_msghdr` is fixed length I guess, so you're able to use `rt + 1` above.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611884887)
I'm not sure, but there reason I wrote this comment is because initially I thought: yikes, what if this is out of bound?
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611884887)
I'm not sure, but there reason I wrote this comment is because initially I thought: yikes, what if this is out of bound?
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611885584)
Remnant of a long-lost pre-C++20 past. Gone.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611885584)
Remnant of a long-lost pre-C++20 past. Gone.
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611888217)
Mmm, I thought perhaps it would return a single error entry to indicate failure.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611888217)
Mmm, I thought perhaps it would return a single error entry to indicate failure.
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2127404990)
I've dropped the dependency on #29625; there is a measurable slowdown from using the (ChaCha20-based) FastRandomContext over the (xoroshiro128++-based) InsecureRandomContext introduced there, but it's no more than 1-2%. I can switch back to that approach if 29625 were to make it in.
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2127404990)
I've dropped the dependency on #29625; there is a measurable slowdown from using the (ChaCha20-based) FastRandomContext over the (xoroshiro128++-based) InsecureRandomContext introduced there, but it's no more than 1-2%. I can switch back to that approach if 29625 were to make it in.
💬 murchandamus commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1611889254)
There is either too many or too few words here:
```suggestion
// This module enforces rules for BIP 431 TRUC transactions (with nVersion=3) which help make
```
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1611889254)
There is either too many or too few words here:
```suggestion
// This module enforces rules for BIP 431 TRUC transactions (with nVersion=3) which help make
```
💬 murchandamus commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1611894367)
Good idea to add this!
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1611894367)
Good idea to add this!
🤔 murchandamus reviewed a pull request: "policy: bump TX_MAX_STANDARD_VERSION to 3"
(https://github.com/bitcoin/bitcoin/pull/29496#pullrequestreview-2074297269)
Still looks good to me
crACK e41dae322d435cd8b32daf73883b466f30349584
(https://github.com/bitcoin/bitcoin/pull/29496#pullrequestreview-2074297269)
Still looks good to me
crACK e41dae322d435cd8b32daf73883b466f30349584
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611896435)
`oldlenp=` does because it's key to the trick of how we get the length first. I find these variable name hints useful to e.g. search for them.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611896435)
`oldlenp=` does because it's key to the trick of how we get the length first. I find these variable name hints useful to e.g. search for them.
👍 GeorgeTsagk approved a pull request: "policy: restrict all TRUC (v3) transactions to 10kvB"
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2074316856)
ack https://github.com/bitcoin/bitcoin/pull/29873/commits/154b2b2296edccb5ed24e829798dacb6195edc11
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2074316856)
ack https://github.com/bitcoin/bitcoin/pull/29873/commits/154b2b2296edccb5ed24e829798dacb6195edc11
💬 luke-jr commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2127412227)
>did that. cleaner and atomic. thanks.
Hmm, it's not that way in your new push tho
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2127412227)
>did that. cleaner and atomic. thanks.
Hmm, it's not that way in your new push tho