💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2439231419)
Just for reference:
issue: https://github.com/bitcoin/bitcoin/issues/33643
fix: https://github.com/bitcoin/bitcoin/pull/33644

💬 rkrux commented on pull request "rpc, test: fix "internal" label check in importdescriptors and extend test":
(https://github.com/bitcoin/bitcoin/pull/33614#issuecomment-3414878717)
Thanks for raising this and highlighting the previous PR as well, which I have reviewed and ACKed. I'm inclining to let this bug be fixed in that PR as that fix seems comprehensive.

💬 maflcko commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3415069851)
> Given that the `len` argument is 0, `memcpy()` should not try to dereference the maybenull argument

That is right and it was already fixed upstream, see also https://github.com/bitcoin/bitcoin/issues/32016#issuecomment-2759479042.

Historically this was a GCC bug, but I was using clang, so I wonder why this somehow changed in the wrong direction. I'll try to take a closer look next week.

💬 maflcko commented on issue "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null":
(https://github.com/bitcoin/bitcoin/issues/33643#issuecomment-3415084114)
The sha256 looks right. Maybe the newlines are interacting somehow with your terminal or they are not stripped by your base64?

```
$ echo 'XGFkZAAAAGRkZWXuXP/fcGcqb2hlcirYfg9D/uXc5eXcRZJ55eXl5eXl5eXlIiL19QAFABD3XERc
AVxhYQcAAADl5f//5eVhYWHl5eX//+Xl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl
5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eX/Km8xMTQyMjgxMUMKYWFhYWFhYQAAAAAA
YWFhYWFhYWFhYWFhYWFhe2FhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFh8mWkovx0AAAA
AAAAAGFhYWFhYWFhYWFhgKoL//v/Kv/////l5eXl5f//ZGRy5eX/
...

🤔 janb84 reviewed a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3349513323)
Concept ACK 2bedad455934c9b407329d0bf58521cf24510465

Thanks for incorporating my suggestion (and those form the other reviewers) !

Found one typo in the cmake file

💬 janb84 commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2439352997)
```suggestion
${properties_content}
```

💬 janb84 commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2439343483)
NIT small typo in variable :
`properies_content` -> `properties_content`

```suggestion
set(properties_content)
list(LENGTH arg_PROPERTIES properties_len)
if(properties_len GREATER "0")
set(properies_content " set_tests_properties(\"\${test_name}\" PROPERTIES\n")
math(EXPR num_properties "${properties_len} / 2")
foreach(i RANGE 0 ${num_properties} 2)
math(EXPR value_index "${i} + 1")
list(GET arg_PROPERTIES ${i} name)
list(GET arg_PROPERTIES
...

💬 diegoviola commented on pull request "Fix Wayland visual glitches":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3415191103)
> If you can, please check how this behaves on #817.

Have you thought about using `requestActivate()` for #817? It seems like the right kind of thing for such a feature.

💬 vasild commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3415282002)
> Historically this was a GCC bug, but I was using clang, so I wonder why this somehow changed in the wrong direction.

I think this is not so much about the compiler but about the nonnull attribute found in `/usr/include/string.h`, which is used by both gcc and clang:

```
/* Copy N bytes of SRC to DEST. */
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
size_t __n) __THROW __nonnull ((1, 2));
```
FreeBSD does not have such a nonnull at
...

💬 vasild commented on issue "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null":
(https://github.com/bitcoin/bitcoin/issues/33643#issuecomment-3415300937)
If the checksum of the binary, base64 decoded, stuff is the same then I must have done it properly... or at least in the same way as you :)

A possible explanation why this might only be observed in some platforms: https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3415282002

👍 vasild approved a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3350029842)
ACK 03ce73a820b485fbdbb854d9d3947ff1e6d87880

💬 vasild commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2439778599)
With the changes in this PR, how would the below code look like?

```py
self.log.info("Sending a pair of transactions with the same txid but different valid wtxids via RPC")
txgen = ValidWitnessMalleatedTx()
funding = wallet.get_utxo()
fee_sat = 1000
siblings_parent = txgen.build_parent_tx(funding["txid"], amount=funding["value"] * COIN - fee_sat)
sibling1, sibling2 = txgen.build_malleated_children(siblings_parent.txid_hex, amount=siblings_
...

💬 vasild commented on issue "Error: Cannot resolve -bind address: 'bitcoind:8334=onion'":
(https://github.com/bitcoin/bitcoin/issues/26484#issuecomment-3415416644)
@dayer3, so Tor daemon is running on another machine and you want to specifically use `-bind=hostname:8334=onion` in `bitcoind` config and not `-bind=a.b.c.d:8334=onion`? I am not sure if that is possible. This comment contains some workaround:

https://github.com/bitcoin/bitcoin/issues/26484#issuecomment-1311427984

💬 instagibbs commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#discussion_r2439936432)
maybe?

```suggestion
"The package must consist of either: exactly 1 transaction, or 1 transaction with its unconfirmed parents. None of the parents may depend on each other. Not all in-mempool parents need to be present.\n"
```

✅ waketraindev closed a pull request: "rpc, test: fix "internal" label check in importdescriptors and extend test"
(https://github.com/bitcoin/bitcoin/pull/33614)

💬 glozow commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#discussion_r2440039778)
Incorporated this as (some, all, or none of)

💬 glozow commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#discussion_r2440040161)
Took this, but reworded a little bit.

💬 glozow commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3415634149)
Thanks for the reviews! Re-pushed to address comments.

💬 ismaelsadeeq commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3415649155)
re-ACK a755e00a13541b3b5a707cf385f1cbec0449c6a9 per https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3409324684

💬 ismaelsadeeq commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2440053480)
@TheCharlatan per https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3409324684 you replace mentioned you add an assertion?

It seems not? it's okay with me though since the handle check for nullptr and throw.