💬 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 ?

💬 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?]."

💬 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.

💬 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?

🤔 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
...

💬 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.

👍 dergoegge approved a pull request: "fuzz: add target for `DescriptorScriptPubKeyMan`"
(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
...

💬 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.

💬 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.

💬 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.

💬 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.

🤔 furszy reviewed a pull request: "test: Add gettransaction test for "coin-join" tx"
(https://github.com/bitcoin/bitcoin/pull/18919#pullrequestreview-1746552710)
utACK fa20f8919c4f

💬 furszy commented on pull request "test: Add gettransaction test for "coin-join" tx":
(https://github.com/bitcoin/bitcoin/pull/18919#discussion_r1403442586)
Could we add a "todo:" at the beginning?
Just to make easier to find broken things in the future.

💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1824513479)
@Sjors testing it includes making sure nothing below minrelay can enter the mempool, unfortunately until v3(where 0-fee will be allowed) or cluster mempool is deployed, which makes trimming very fast

💬 instagibbs commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1824515731)
@Sjors I think you can set `-minrelaytxfee=0` to work around it?

💬 maflcko commented on pull request "test: Add gettransaction test for "coin-join" tx":
(https://github.com/bitcoin/bitcoin/pull/18919#discussion_r1403457573)
I am not a fan of TODO comments, because I think issues in the issue tracker are a better way to track outstanding feature requests and bugfixes. An issue allows anyone to leave new context, ask questions, or look up the full discussion history on the topic.

So going to leave as-is for now.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1403481849)
Do you remember why you added a `do { ... } while (0)` to the `TRACEPOINT` macro?

💬 Sjors commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1824570747)
> @Sjors I think you can set `-minrelaytxfee=0` to work around it?

That doesn't simulate a full mempool though.

I wrote a commit that lets my example go through: https://github.com/Sjors/bitcoin/commit/03baacd7643daf3d1c8efbc1a723719d8cc72dce

However, not sure if it's worth the extra complexity.

🤔 Sjors reviewed a pull request: "bugfix, Change up submitpackage results to return results for all transactions"
(https://github.com/bitcoin/bitcoin/pull/28848#pullrequestreview-1746652527)
Anyway, my two main issues are about pre-existing behavior: `tx-results` was already a dictionary and `-minrelaytxfee` already behaved that way.

d72aab3dbd8847e851f8b724b195181e30e7bf02 looks good, but I'm (obviously) not familiar enough with mempool stuff to know for sure.