💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1124908789)
Updated

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1125071450)
Thanks, I’m not going to interfere here, because outpoints are tiny anyway, and it seems more readable without the move or constructor.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1124956187)
Thanks that’s much nicer

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1125071911)
I’ll have to revisit this one.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1124966759)
It seems to me that if we don’t use the result of `find`, it’s clearer that we just care about whether a key is present instead of where in the sequence it appears

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1454249283)
Rebased on latest version of #27021

👍 yahiheb approved a pull request: "Fix typos in comments to make linter happy"
(https://github.com/bitcoin/bitcoin/pull/27197)

👍 kristapsk approved a pull request: "Fix typos in comments to make linter happy"
(https://github.com/bitcoin/bitcoin/pull/27197)
utACK 987f1bb41c0a8c54422066e10d1c63e19c4df66d

🚀 fanquake merged a pull request: "Fix typos in comments to make linter happy"
(https://github.com/bitcoin/bitcoin/pull/27197)

👍 fanquake approved a pull request: "util: add missing include and fix function signature"
(https://github.com/bitcoin/bitcoin/pull/27192)
ACK 8847ce44e0713350a6d3524f62eaeb10ba548bae

🚀 fanquake merged a pull request: "util: add missing include and fix function signature"
(https://github.com/bitcoin/bitcoin/pull/27192)

💬 fanquake commented on issue "Adoption of BIP39/44/49/84, and classification of (extended) pub/priv keys, addresses, mnemonics, etc":
(https://github.com/bitcoin/bitcoin/issues/17748#issuecomment-1454608242)
cc @achow101

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1454642004)
Updates:
- Cleanup from previous approach.
- Closing all dialogs with transaction details that were opened from the transactions view when mask values is selected.

💬 mauri146K commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-1454644421)
> There is the irrelevant error message output by the signrawtransactionwithkey command.
>
> **Expected behavior**
>
> Hex string of the raw transaction with signature OR meaningful message about an alternative way to achieve one.
>
> **Actual behavior**
>
> ```
> {
> "hex": "0200000001a2c0d82460883696219dbca6f545f72963b2b3ee085d832eb5ef9a69a374af160000000000fdffffff01e011000000000000225120052e44f45a6e381be8e06d3f3362b58034a68ba98081e24de7bfc5795420a90b00000000",
> "complete": false,
>
...

💬 fanquake commented on pull request "doc: Expand scantxoutset help text to cover tr() and miniscript":
(https://github.com/bitcoin/bitcoin/pull/27155#issuecomment-1454659148)
> Should we version the doc/descriptors.md file? By that I mean literally add details in the doc saying when specific formats were added.

cc @achow101

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1124914676)
I think this condition is guaranteed by the other static assertions, but this expression seems incorrect. In general, `A & (B-1) == 0` doesn't mean that A is a multiple of B, does it? If A=8 and B=3, then `8 & (3-1)` is zero, but 8 isn't a multiple of 3. If A=8, B=4, then `8 & (4-1)` is also zero, so we get the correct result (8 is a multiple of 4), but it's kind of by accident. To state it differently, if `MAX_BLOCK_SIZE_BYTES` is some large power of 2 (which is guaranteed above), then in binar
...

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125417231)
I don't understand the purpose of adding `sizeof(void*) * 4`; could you leave a brief comment if you get the chance? (Unless I'm just being clueless!)

💬 MarcoFalke commented on pull request "util: add missing include and fix function signature":
(https://github.com/bitcoin/bitcoin/pull/27192#discussion_r1125426076)
Yeah, I was mostly thinking that it would be good to check for other missing includes while touching the file, so that it doesn't have to be touched again later if there is a missing stdlib include or so.

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443783)
In line 89 there is an assert that `ELEM_ALIGN_BYTES` is multiple of 2:
```cpp
static_assert((ELEM_ALIGN_BYTES & (ELEM_ALIGN_BYTES - 1)) == 0, "ELEM_ALIGN_BYTES must be a power of two");
```
So given that line 91 should make sense, `ELEM_ALIGN_BYTES - 1` becomes a bitmask and actually asserts that `MAX_BLOCK_SIZE_BYTES` is multiple of `ELEM_ALIGN_BYTES`. But right, on its own that assert wouldn't be enough and using `%` is probably a bit more clear.

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443786)
Posting here now, I can add this as a comment later:

The value here determines the maximum bytes that the `PoolAllocator` supports. When bigger blocks are allocated, this is just forwarded to `new`.

The thing with `sizeof(void*) * 4` is, it is not enough to just support up to sizes of the `std::pair`, because the different implementations of `std::unordered_map` use more memory for each node. Most implementations wrap the std::pair into a struct that contains a single pointer, so they can
...