💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159666836)
confirmed this does fix the download selection filter

💬 MarcoFalke commented on pull request "ci: Run base install at most once":
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159667906)
Yes, `DANGER_RUN_CI_ON_HOST` is called that way to discourage people from trashing their system

💬 MarcoFalke commented on pull request "ci: Run base install at most once":
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159668508)
(This applies to all `CI_EXEC` lines, so I don't think anything is gained from mentioning it here)

🤔 TheCharlatan reviewed a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279)
Approach ACK

💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159670307)
nit: it would be nice to have it also print out the destination, e.g "downloading {binary_filename} to {destination}"

💬 glozow commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1499006887)
Concept ACK. I agree there should be a mailing list post.

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1499080469)
> > Anyway the data from [miningpool.observer/template-and-block](https://miningpool.observer/template-and-block) shows that blocks routinely include a dozen or so unexpected transactions. It's not clear if those are transactions not in miningpool.observer's mempoo, and thus relevant to compact blocksl; @0xB10C, can you clarify this point?
>
> Transactions listed under "Extra Transactions" are transactions that weren't in my mempool but the pool knew about. These are transactions my node woul
...

🤔 glozow reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145)
Concept ACK

There is a missing dash in "Co-authored-by:" in the commit messages

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159806134)
Sorry to bikeshed, but I think the naming is a bit hard to follow ("conflicted" vs "conflicting" don't immediately make sense to me, especially since both sets of transactions get conflicted at some point during the test). Might I suggest names that indicate which input(s) they spend, which node broadcasts them, and what "generation" they are? For example

`tx_AB_0_parent` instead of `conflicted`
`tx_A_1` instead of `conflicting_tx_A`
`tx_B_1` instead of `conflicting_tx_B`

`C` instead of
...

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159787578)
nit in 77a2e522d845eda270b2c03fd7e0a16bc4c00fbb, I think this is clearer (same with the other arrays of inputs and outputs)

```suggestion
inputs_conflicted_tx = [{"txid": txid_conflict_from_1, "vout": nA}, {"txid": txid_conflict_from_2, "vout": nB}]
```

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159765529)
in 77a2e522d845eda270b2c03fd7e0a16bc4c00fbb:

I think this comment is a little bit hard to follow. Something more descriptive might be
```suggestion
"""
Test that wallet correctly tracks transactions that have been conflicted by blocks, particularly during reorgs.
"""
```

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159807735)
```suggestion
self.sync_blocks()
```

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159814534)
in commit 49f8af533f2e6370f4ae5220f5c706e30b90d06c

no need for const reference to integer

💬 glozow commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1159784792)
We should also test who "wins" in this conflict contest and what the values are, i.e. conflicting A has 8 confirmations and conflicting B has 4 confirmations.

💬 pinheadmz commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1499130274)
> the only solution is to do a full resync and download all the way back to N-N (aka 0).

There is also `getblockfrompeer` https://github.com/bitcoin/bitcoin/pull/20295 I just tried this on my pruned node to inspect blocks below the prune depth, pretty neat!

🤔 furszy reviewed a pull request: "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/25193#pullrequestreview-1375021974)
Instead of the global flag that requires many manual sets at different locations and the indexes threads active wait, what if we move the indexes threads start after the loading process? e.g. https://github.com/furszy/bitcoin-core/commit/1e48179f70dafff8fad31b4048681103cca90b56.

It makes code shorter and more robust. Plus, it let us keep the pruning checks as well.

💬 pinheadmz commented on issue "Log X-Forwarded-For for rpc ":
(https://github.com/bitcoin/bitcoin/issues/9397#issuecomment-1499239728)
I think this issue can be closed as wont-fix. However, I think the only docs we have regarding stunnel are in [the 0.12 release notes](https://github.com/esotericnonsense/bitcoin/blob/b583b3993a0e22d8adfed477077dc9b889f8b2ad/doc/release-notes/release-notes-0.12.0.md#rpc-ssl-support-dropped) and were not sufficient for me trying to follow them (we could mention the stunnel conf file, firewall rules, etc -- not to mention @laanwj suggestions in the above comment).

💬 pinheadmz commented on issue "verifytxoutproof can return witnesses but cannot verify them":
(https://github.com/bitcoin/bitcoin/issues/11242#issuecomment-1499265142)
This is a good idea and seems like something we should support. The output would need to prove the entire coinbase tx and then include a wtxid proof to the witness commitment. `merkleblock` is well-documented but this extra proof might need a BIP spec even if its just for an RPC.

Are there any other uses for merkleblock proofs outside of SPV wallets? There hasn't been any more discussion on this issue for 6 years and we have client-side filtering available now which doesn't require proofs at
...

💬 S3RK commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#issuecomment-1499322028)
ACK 25ab14712b9d80276016f9fc9bff7fb9c1d09635

The only change is my previous comment addressed about using current variable to determine change output size.

💬 pinheadmz commented on issue "MAX_BLOCK_SERIALIZED_SIZE is not a consensus-enforced constant":
(https://github.com/bitcoin/bitcoin/issues/10289#issuecomment-1499351182)
Could the useage sites be switched to `MAX_BLOCK_WEIGHT`?

> an expression that computes it from consensus constants.

What would this expression be besides `MAX_BLOCK_WEIGHT * 1` ?