👍 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.
💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1824356842)
> why not save the xor key inside the leveldb block database?
Just seems more complicated with no benefit?
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1824356842)
> why not save the xor key inside the leveldb block database?
Just seems more complicated with no benefit?
🤔 furszy reviewed a pull request: "test: add coverage for bech32m in `wallet_keypool_topup`"
(https://github.com/bitcoin/bitcoin/pull/28928#pullrequestreview-1746380061)
> > we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.
>
> sgtm, perhaps 2 nodes? why only one?
The test is solely verifying the wallet can (re)generate the key pool. This can be tested by loading and unloading wallets instead of restarting nodes (this was a restriction from the past, prev to introducing multi-wallets support). No extra nodes nor restart is needed.
> Also, I have no problem in addressing it here, but perhaps
...
(https://github.com/bitcoin/bitcoin/pull/28928#pullrequestreview-1746380061)
> > we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.
>
> sgtm, perhaps 2 nodes? why only one?
The test is solely verifying the wallet can (re)generate the key pool. This can be tested by loading and unloading wallets instead of restarting nodes (this was a restriction from the past, prev to introducing multi-wallets support). No extra nodes nor restart is needed.
> Also, I have no problem in addressing it here, but perhaps
...
💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1403335348)
According to https://corecheck.dev/bitcoin/bitcoin/pulls/28892 there were three instances in total, so I went ahead and fixed them all.
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1403335348)
According to https://corecheck.dev/bitcoin/bitcoin/pulls/28892 there were three instances in total, so I went ahead and fixed them all.
👍 dergoegge approved a pull request: "fuzz: add target for `DescriptorScriptPubKeyMan`"
(https://github.com/bitcoin/bitcoin/pull/28578#pullrequestreview-1746391872)
ACK 47e5c9994c087d1826ccc0d539e916845b5648fb
(https://github.com/bitcoin/bitcoin/pull/28578#pullrequestreview-1746391872)
ACK 47e5c9994c087d1826ccc0d539e916845b5648fb
💬 petertodd commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1824382711)
> @petertodd
>
> > Note that large coinbase transactions have caused a number of problems before,
> > including issues with ASIC compatibility and p2pool. We should not inflate
> > coinbase transaction size needlessly.
>
> What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
Obviously we should not do things that are incompatible with existing ASICs without a *very* good reason. A feature that doesn't
...
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1824382711)
> @petertodd
>
> > Note that large coinbase transactions have caused a number of problems before,
> > including issues with ASIC compatibility and p2pool. We should not inflate
> > coinbase transaction size needlessly.
>
> What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
Obviously we should not do things that are incompatible with existing ASICs without a *very* good reason. A feature that doesn't
...