💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611813658)
Also, note that in a non-TRUC 1p1c scenario, the parent's increased descendant size limit is mooted by the ancestor size limit (of the child).
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611813658)
Also, note that in a non-TRUC 1p1c scenario, the parent's increased descendant size limit is mooted by the ancestor size limit (of the child).
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611829012)
i don't think so, the pointer needs to advance by the size of the specific message, which is part of that message.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611829012)
i don't think so, the pointer needs to advance by the size of the specific message, which is part of that message.
💬 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_r1611830313)
@josibake I looked around a bit and there might be something here. `GetHash()` is called before this point
- when we read the block from disk https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/node/blockstorage.cpp#L1092
- when `ActivateBestChain` is called https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/validation.cpp#L3354
There might be some gain in caching GetHash() but I think that has to be addressed on its own. I
...
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611830313)
@josibake I looked around a bit and there might be something here. `GetHash()` is called before this point
- when we read the block from disk https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/node/blockstorage.cpp#L1092
- when `ActivateBestChain` is called https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/validation.cpp#L3354
There might be some gain in caching GetHash() but I think that has to be addressed on its own. I
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611831862)
i'm not sure it makes sense to duplicate documentation that's already in the manual pages. Like if we're doing this here, why not for every system call we make. Do parameter names like `*newp=` `oldp=` even elucidate much?
Agree re using `nullptr` where possible.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611831862)
i'm not sure it makes sense to duplicate documentation that's already in the manual pages. Like if we're doing this here, why not for every system call we make. Do parameter names like `*newp=` `oldp=` even elucidate much?
Agree re using `nullptr` where possible.
💬 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_r1611833798)
The `TxStateBlockConflicted` requires both that's why I added the both of them
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611833798)
The `TxStateBlockConflicted` requires both that's why I added the both of them
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611834614)
Yes, it's a minor optimization, we could always construct a CNetAddr if we wanted, but if we know we're not going to use it anyway might as well skip it.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611834614)
Yes, it's a minor optimization, we could always construct a CNetAddr if we wanted, but if we know we're not going to use it anyway might as well skip it.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611838998)
Well, i guess it wouldn't hurt checking (or adding an `Assume`), but i don't think the routing table is supposed to contain error entries.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611838998)
Well, i guess it wouldn't hurt checking (or adding an `Assume`), but i don't think the routing table is supposed to contain error entries.
🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2074201852)
ACK 63cfa4ce88
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2074201852)
ACK 63cfa4ce88
💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611833441)
Is there a reason for `struct` (seems like a C thing?)
```suggestion
SubPackageState m_subpackage;
```
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611833441)
Is there a reason for `struct` (seems like a C thing?)
```suggestion
SubPackageState m_subpackage;
```
💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611842178)
Seems this will need to be deleted immediately in the next PR?
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611842178)
Seems this will need to be deleted immediately in the next PR?
💬 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_r1611846940)
> Any reason why `conflicting_block_hash` is passed by reference but `replacement_tx` isn't?
`CTransactionRef` is already a pointer https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/primitives/transaction.h#L423-L424
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611846940)
> Any reason why `conflicting_block_hash` is passed by reference but `replacement_tx` isn't?
`CTransactionRef` is already a pointer https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/primitives/transaction.h#L423-L424
🚀 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
...