💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1620650118)
Left a new round of reviews, I think the following conceptual concerns are pending:
- padding of the signaling byte to preserve evolvability for future consensus validation of the annex data
- adding a `MAX_ANNEX_BUDGET` or not to mitigate potential CPU DoS concerns for full-nodes, and potential impact for fee-bumping reserves
- opted-in of `annexrelay` for node operators
- annex policy limits versioned on the transaction’s `nVersion` field

I still have to reply on the latest mail post ab
...

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1252319116)
ACK, please resolve.

💬 dimitaracev commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1620693065)
ACK `5cf4427`

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1252331248)
> I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating `m_undo_height_in_last_blockfile` when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.

It's not surprising that there's no test coverage for this. But to explain: the goal of the code is to flush the undo file when t
...

💬 JayBitron commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252364554)
`compare` throws an exception if the input string is lower than 4 character, but since this a conflict PR, please close it.

💬 ismaelsadeeq commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252358645)
```c++
if (parent_feerate < individual_feerate) _removeTx(parent.GetTx().GetHash(), /* inBlock = */true);
```
This means parents that are confirmed from previous blocks are going to be removed as well right? not just parents from this block we are processing?

💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620768425)
Concept ACK

👍 jarolrod approved a pull request: "Add menu option to migrate a wallet"
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1513684449)
ACK 48aae2cffeb91add75a70ac4d5075c38054452fa

This introduces the functionality as promised, works well, and didn't have any issues with legacy wallets + watch only addrs.

Improving the text and it's translatability to can occur in a follow-up.

🤔 jarolrod reviewed a pull request: "Disable and uncheck blank when private keys are disabled"
(https://github.com/bitcoin-core/gui/pull/739#pullrequestreview-1513716076)
ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177

This is correct, and an improvement.

Visual and UX improvements on wallet creation can be handled for a follow-up here if someone wants to take it. The issue around UX of wallet creation will be tackled in https://github.com/bitcoin-core/gui-qml

🤔 jarolrod reviewed a pull request: "Show own outputs on PSBT signing window"
(https://github.com/bitcoin-core/gui/pull/740#pullrequestreview-1513819941)
ACK 4da243ba023f2987e97fc62886c6ebc70d6ee50a

Tested for functionality, and confirmed new translation added here shows up when building translations.

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252663074)
I am not sure if I fully get what you mean. Can you give a concrete example of a future extended format that might interfere with the 0x00 + unstructured proposal? My thought was to simply reserve all non-0x00 first bytes for future use.

When adding 32 bytes of padding, do you mean that initially (in a world with only unstructured annex data), 31 of those bytes are wasted but paid for?

💬 jarolrod commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1621182702)
Concept ACK

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252685430)
>I don’t think this CPU DoS risk, if correct, is that much an increase in DoS surface, though a MAX_ANNEX_BUDGET can be a good conservative “belt-and-suspender” DoS limits, like we have a lot in Bitcoin Core.

If it isn't that much of an increase, is it worth adding extra code for it? More code, more things to go wrong. In the end, they are also paying for this tx in sats and the computation cost only increases linearly with the number of inputs.

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252689789)
Yes, that's clear now. Thanks.

Same as with `MAX_ANNEX_BUDGET`, my feeling is that this safeguard isn't necessary because the extra cost is only linear and paid for.

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252693892)
>If you’re taking the use-case of unstructured data discussed on the mailing list

Which one do you refer to exactly, the timelocked vaults using presigned txes? For that there is only a single party, so the inflation attack doesn't apply?

💬 virtu commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621225530)
Even with the extra information, I'm somewhat at a loss because I cannot reproduce the problem locally. From what I understand:
- the tracepoint got called, the handler was executed
- the tx hash is correct (so `bpf_usdt_readarg_p(1, ctx, &rejected.hash, HASH_LENGTH)` in `trace_rejected` works as expected)
- reject reason is usually correct; but sometime's it's an empty string, suggesting something went wrong with `bpf_usdt_readarg_p(2, ctx, &rejected.reason, MAX_REJECT_REASON_LENGTH)` in `tr
...

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1621228405)
>I think at that stage the proposal would benefit from an “applications" BIP draft that would give one or two use-cases in detail, and therefore serves as technical tie-breaker for the conceptual concerns raised above.

I agree that use cases can help guide the decision making in this PR. That's why I created the demo/explanation mentioned in the PR description: https://github.com/joostjager/annex-covenants.

💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621281848)
Looking at the code, I fail to see how this could even pass once, because you can't assume a pointer created by `std::string{}.c_str()` can be used after the statement at all, no?

📝 fanquake locked a pull request: "typos on src files"
(https://github.com/bitcoin/bitcoin/pull/28022)
Fixes multiple typos:

efficently --> efficently
succesfully --> successfully
coeffients --> coefficients
determinstic --> deterministic
initalized --> initialized
occurences --> occurrences
implemenation --> implementation

💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621292840)
Or do the `TRACEx` macros create a copy of the data in-line? If yes, it could make sense to document that in doc/tracing.md#c_str