💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403157275)
^^ I ran HEAD~2 by mistake when reviewing this.
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403157275)
^^ I ran HEAD~2 by mistake when reviewing this.
💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403159073)
There are two files. `arith_uint256_tests.cpp`, which mostly tests the `arith*` class, and `uint256_tests.cpp`, which mostly tests the `uint*` class.
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403159073)
There are two files. `arith_uint256_tests.cpp`, which mostly tests the `arith*` class, and `uint256_tests.cpp`, which mostly tests the `uint*` class.
💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403160611)
I think it is fine to call construction from a string "string constructor", but may change if I have to re-touch again for other reasons.
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403160611)
I think it is fine to call construction from a string "string constructor", but may change if I have to re-touch again for other reasons.
👍 dergoegge approved a pull request: "multiprocess: Add basic type conversion hooks"
(https://github.com/bitcoin/bitcoin/pull/28921#pullrequestreview-1746109544)
Code review ACK a506148b7a7058b4c60e5c497e70688134ee9a3f
CI failure looks spurious.
(https://github.com/bitcoin/bitcoin/pull/28921#pullrequestreview-1746109544)
Code review ACK a506148b7a7058b4c60e5c497e70688134ee9a3f
CI failure looks spurious.
💬 TheCharlatan commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403172882)
They're not really testing `arith_uint256` anymore though, just the new conversion helper. I don't feel strongly about this, these are just a bunch of test lines after all, but was curious what the criteria was here.
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403172882)
They're not really testing `arith_uint256` anymore though, just the new conversion helper. I don't feel strongly about this, these are just a bunch of test lines after all, but was curious what the criteria was here.
👍 TheCharlatan approved a pull request: "refactor: Remove unused and fragile string interface from arith_uint256"
(https://github.com/bitcoin/bitcoin/pull/28924#pullrequestreview-1746132253)
ACK fa63f16018d9468e1751d2107b5102184ac2d5ae
(https://github.com/bitcoin/bitcoin/pull/28924#pullrequestreview-1746132253)
ACK fa63f16018d9468e1751d2107b5102184ac2d5ae
💬 Sjors commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1824139761)
Concept ACK
The RPC help is a bit abstract at the moment, e.g. "Results may optionally be grouped so only certain dimensions are shown."
It might be enough to only show 1 `Result:` section in the help, e.g. just with two dimensions.
In the example section (and in the PR description), you could add a description, e.g.:
* Get the total number of messages and bytes sent/received: `bitcoin-cli getnetmsgstats`
* Show number of messages and bytes per message type: `bitcoin-cli getnetmsgs
...
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1824139761)
Concept ACK
The RPC help is a bit abstract at the moment, e.g. "Results may optionally be grouped so only certain dimensions are shown."
It might be enough to only show 1 `Result:` section in the help, e.g. just with two dimensions.
In the example section (and in the PR description), you could add a description, e.g.:
* Get the total number of messages and bytes sent/received: `bitcoin-cli getnetmsgstats`
* Show number of messages and bytes per message type: `bitcoin-cli getnetmsgs
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403197577)
flipped the `<` and flipped the ternary conditions. The behavior remains the same. Indeed, it was double-upside-down before.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403197577)
flipped the `<` and flipped the ternary conditions. The behavior remains the same. Indeed, it was double-upside-down before.
💬 maflcko commented on pull request "refactor: Remove unused and fragile string interface from arith_uint256":
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403198051)
Yeah, this line is checking the `arith_uint::operator==`, or maybe a test self-sanity-check? Seems fine to keep when in doubt, I guess.
(https://github.com/bitcoin/bitcoin/pull/28924#discussion_r1403198051)
Yeah, this line is checking the `arith_uint::operator==`, or maybe a test self-sanity-check? Seems fine to keep when in doubt, I guess.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403205396)
I think this stuff remained from the legacy code where its use was encapsulated.... just deleting the comments.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403205396)
I think this stuff remained from the legacy code where its use was encapsulated.... just deleting the comments.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403207296)
eh, it's just a workaround to handle almost-0-fractional-value C++ thing i guess. Might as well be 0.0001 or 0.000000001. Not sure how to make it beautiful.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403207296)
eh, it's just a workaround to handle almost-0-fractional-value C++ thing i guess. Might as well be 0.0001 or 0.000000001. Not sure how to make it beautiful.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403216806)
it will have to be moved in the following PR, but yeah, I'll stick to your suggestion in this one.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403216806)
it will have to be moved in the following PR, but yeah, I'll stick to your suggestion in this one.
💬 fanquake commented on issue "Release schedule for 26.0":
(https://github.com/bitcoin/bitcoin/issues/27758#issuecomment-1824235892)
> Should the release schedule be updated?
Yes. An [rc3](https://github.com/bitcoin/bitcoin/releases/tag/v26.0rc3) was just tagged, so it will need to be pushed back a week or two.
(https://github.com/bitcoin/bitcoin/issues/27758#issuecomment-1824235892)
> Should the release schedule be updated?
Yes. An [rc3](https://github.com/bitcoin/bitcoin/releases/tag/v26.0rc3) was just tagged, so it will need to be pushed back a week or two.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403256259)
We call this for every peer. So there might be, say, 8 calls for the same `wtxid`. The risk of changing `m_states` i thought is acceptable, it's a rare event (outbound peers) and hard to exploit in a meaningful way. But I'm open to more analysis.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403256259)
We call this for every peer. So there might be, say, 8 calls for the same `wtxid`. The risk of changing `m_states` i thought is acceptable, it's a rare event (outbound peers) and hard to exploit in a meaningful way. But I'm open to more analysis.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403265334)
Sure, what exactly?
here I talk about an alternative way to implement this.
Say, we want to add a child to the set, because the parent is already there, and we don't want child ahead of parent. But the set is full. We can't ignore the limit — that's (a). (b) means that we could fanout child+parent in this case, but this `remove parent from set` operation is harder to reason about. Should we then remove parents of a parent too?
Maybe I overthink this issue.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1403265334)
Sure, what exactly?
here I talk about an alternative way to implement this.
Say, we want to add a child to the set, because the parent is already there, and we don't want child ahead of parent. But the set is full. We can't ignore the limit — that's (a). (b) means that we could fanout child+parent in this case, but this `remove parent from set` operation is harder to reason about. Should we then remove parents of a parent too?
Maybe I overthink this issue.
💬 dergoegge commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1403275532)
Yes the plan is to remove it eventually, but it should still be needed at the moment.
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1403275532)
Yes the plan is to remove it eventually, but it should still be needed at the moment.
🤔 Sjors reviewed a pull request: "bugfix, Change up submitpackage results to return results for all transactions"
(https://github.com/bitcoin/bitcoin/pull/28848#pullrequestreview-1746269146)
Mmm, excuse my ignorance, but shouldn't the minimum relay fee be ignored for packages?
```
src/bitcoin-cli -signet submitpackage '["02000000000101f7307b92590b1a7d82e878258a62ffaad05d0db510e947c3c331fe7400469cac0000000000fdffffff029d0c05020000000016001453dcf718842726f5236d0c05b1a5d43e12097731a086010000000000160014acdaaab857365e45724557f835694bdbb0c81231014009e5f00ec66eb80f497b771e2eb10cff222c60dc6b1fe865e5371142b1a02af7fbef913b0bc3bd49be679240e3c87bcde58532e53f657f9805352a43da036c965d9a0200",
...
(https://github.com/bitcoin/bitcoin/pull/28848#pullrequestreview-1746269146)
Mmm, excuse my ignorance, but shouldn't the minimum relay fee be ignored for packages?
```
src/bitcoin-cli -signet submitpackage '["02000000000101f7307b92590b1a7d82e878258a62ffaad05d0db510e947c3c331fe7400469cac0000000000fdffffff029d0c05020000000016001453dcf718842726f5236d0c05b1a5d43e12097731a086010000000000160014acdaaab857365e45724557f835694bdbb0c81231014009e5f00ec66eb80f497b771e2eb10cff222c60dc6b1fe865e5371142b1a02af7fbef913b0bc3bd49be679240e3c87bcde58532e53f657f9805352a43da036c965d9a0200",
...
💬 Sjors commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403273829)
consist of parents followed by exactly one child,
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403273829)
consist of parents followed by exactly one child,
💬 Sjors commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403265805)
a -> any ?
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403265805)
a -> any ?
💬 Sjors commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403281170)
Maybe add: "A transaction that's already in the mempool is [ignored, rebroadcast?]."
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1403281170)
Maybe add: "A transaction that's already in the mempool is [ignored, rebroadcast?]."
💬 lrs955 commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1824353131)
why not save the xor key inside the leveldb block database?
> > wouldn't it be better to save the key in the .conf or settings?
>
> The key is not a configuration or setting that is supposed to be modified by a user or that can be changed at all. Once it is persisted to storage, it is set in stone for all (future) dat files.
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1824353131)
why not save the xor key inside the leveldb block database?
> > wouldn't it be better to save the key in the .conf or settings?
>
> The key is not a configuration or setting that is supposed to be modified by a user or that can be changed at all. Once it is persisted to storage, it is set in stone for all (future) dat files.