💬 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
...
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403356154)
Sure. Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403356154)
Sure. Done as suggested.
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403366367)
Yeah, I was thinking about https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401323753 and other features I have in mind. But, more generally, I believe the wallet should be aware of the source of the event. However, upon further thought, we could achieve this by creating multiple handlers on the wallet side instead of passing the origin pointer to the signal itself.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1403366367)
Yeah, I was thinking about https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1401323753 and other features I have in mind. But, more generally, I believe the wallet should be aware of the source of the event. However, upon further thought, we could achieve this by creating multiple handlers on the wallet side instead of passing the origin pointer to the signal itself.
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1824411647)
Updated per feedback. Tiny [diff](https://github.com/bitcoin/bitcoin/compare/44e2c744b8bae075da69d8e8dd392296899a68a5..25de6ff824bffd8617a6baca19bbe57d6764a9d2).
Only added a small explanatory comment.
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1824411647)
Updated per feedback. Tiny [diff](https://github.com/bitcoin/bitcoin/compare/44e2c744b8bae075da69d8e8dd392296899a68a5..25de6ff824bffd8617a6baca19bbe57d6764a9d2).
Only added a small explanatory comment.
💬 Sjors commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1824503673)
Just ran into this today while testing #27463.
To make testing of package submission easier, should we have an option to allow this on test networks? Or perhaps a way to set a minimum for `GetMinFee()`?
@sdaftuar does mempool clustering fix this? E.g. is it just a matter of (occasionally) evicting chunks below `-minrelaytxfee`? From the above discussion I get the impression that the answer is yes, but most of this was discussed before that proposal came out.
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1824503673)
Just ran into this today while testing #27463.
To make testing of package submission easier, should we have an option to allow this on test networks? Or perhaps a way to set a minimum for `GetMinFee()`?
@sdaftuar does mempool clustering fix this? E.g. is it just a matter of (occasionally) evicting chunks below `-minrelaytxfee`? From the above discussion I get the impression that the answer is yes, but most of this was discussed before that proposal came out.
🤔 furszy reviewed a pull request: "test: Add gettransaction test for "coin-join" tx"
(https://github.com/bitcoin/bitcoin/pull/18919#pullrequestreview-1746552710)
utACK fa20f8919c4f
(https://github.com/bitcoin/bitcoin/pull/18919#pullrequestreview-1746552710)
utACK fa20f8919c4f